-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Align sections job #7820
Align sections job #7820
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this new job 🙏
Please find my refacoring suggestions below :)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend looks good (see micha’s suggestions about error messages though)
Where does the align_example.jpg come from? Did you double-check that we may publish this data? Do the left and right side really point to the same data? (The blood vessel on the right is so bright, I would have expected to see something of that in the non-aligned version too) Also, maybe png would be better suited to avoid the compression artifacts? That’s not super important though.
The pictures can also be found on our website (https://scalableminds.com/alignment-service) so that should not be a problem.
It is not the same picture on the left and on the right but rather one dataset with the left side showing the left section of the dataset in the unaligned state and the right side showing the right section of the dataset in the aligned state. |
Ok, yes these were checked :)
Fair enough. I would have expected a before→after image, of the same crop, but it’s not very important. Feel free to leave it as is for the moment, maybe we’ll insert a more refined image later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for applying my suggestions 🦖
I found a few other things, which I did not notice in the first review round. Please also fix them. Then the code should be fine :)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: MichaelBuessemeyer <39529669+MichaelBuessemeyer@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frontend code looks good to me 🎉
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)