-
Notifications
You must be signed in to change notification settings - Fork 5
Add binning for fusion #15
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
base: main
Are you sure you want to change the base?
Add binning for fusion #15
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
- Coverage 86.24% 81.09% -5.15%
==========================================
Files 13 13
Lines 858 873 +15
==========================================
- Hits 740 708 -32
- Misses 118 165 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Edit: Just added a test, ran locally and realized that tests are probably running only on ubuntu and macOS for a reason. Committing and 🤞 that it's working in the CI... |
m-albert
left a comment
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.
Hey @jo-mueller thanks a lot for this PR! Added a few comments.
|
@m-albert Should hopefully be alright now 👍 |
|
Thanks for your work here @jo-mueller. Will have a look at this next week! |
|
No hurries, it's not at all time-critical. Enjoy your vacations! |
m-albert
left a comment
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.
Hey @jo-mueller, sorry for the delay in reviewing here.
Thanks again for the PR! I reviewed and left some comments on the changes.
|
|
||
| # check which keys are in spacing that are missing in fusion_binning and add them | ||
| if self.custom_fuse_binning.value: | ||
| fusion_binning = {'y': self.x_fuse_binning.value, 'x': self.x_fuse_binning.value} |
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.
Probably this would be self.y_fuse_binning.value for y!
| fusing_spacing = spatial_image_utils.get_spacing_from_sim(sims[0]) | ||
| fusing_spacing = { | ||
| key: fusing_spacing[key] * fusion_binning[key] | ||
| for key in fusing_spacing.keys() if key in fusion_binning |
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.
In the case of 3D input data, fusing_spacing would currently loose the 'z' key (I tink best is to just keep it).
| stitcher_widget.run_fusion() | ||
|
|
||
| # turn on custom binning | ||
| stitcher_widget.custom_reg_binning.value = True |
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.
I think it would be great to iterate through both reg and fus binning on and off in the tests.
Fixes #13
Hi @m-albert ,
the changes to bring this feature into the plugin were actually quite minimal :) I took the liberty of renaming the binning option in the advanced settings tab so that they'd be valid for both registration and fusion. Otherwise, unlike the registration call, the
fusecommand required the target spacing in all axes, so I had to gather thez-spacingand paste it into theoutput_spacing.Let me know what you think :)