Skip to content
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

Merged
merged 20 commits into from
Jun 11, 2024
Merged

Align sections job #7820

merged 20 commits into from
Jun 11, 2024

Conversation

Tobias314
Copy link
Contributor

@Tobias314 Tobias314 commented May 23, 2024

Steps to test:

  • Checkout the corresponding vocxelytics PR and start the worker. Afterwards, open a dataset in you local Webknossos and select alignment from the AI tools

Issues:


(Please delete unneeded items, merge only when none are left open)

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a 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 :)

app/controllers/JobsController.scala Outdated Show resolved Hide resolved
app/controllers/JobsController.scala Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@fm3 fm3 left a 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.

@Tobias314
Copy link
Contributor Author

Where does the align_example.jpg come from? Did you double-check that we may publish this data?

The pictures can also be found on our website (https://scalableminds.com/alignment-service) so that should not be a problem.

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).

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.

@fm3
Copy link
Member

fm3 commented Jun 3, 2024

The pictures can also be found on our website so that should not be a problem.

Ok, yes these were checked :)

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.

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.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a 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 :)

Co-authored-by: MichaelBuessemeyer <39529669+MichaelBuessemeyer@users.noreply.github.com>
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a 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 🎉

@Tobias314 Tobias314 merged commit 3b8e545 into master Jun 11, 2024
2 checks passed
@Tobias314 Tobias314 deleted the align-sections-job branch June 11, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants