-
Notifications
You must be signed in to change notification settings - Fork 2
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
Basic functionality #14
Conversation
…ister [IBL] or ARA)
…and perform selected transforms with either elastix defaults or provided ARA defaults
…functionality # Conflicts: # src/bg_elastix/_widget.py
for more information, see https://pre-commit.ci
This is my plan for now, after I get all of this working I think we can move on to damaged slice, and registering sub-volumes |
src/bg_elastix/elastix/register.py
Outdated
|
||
|
||
def run_registration( | ||
fixed_image, | ||
brain_atlas, |
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.
Would it be better to keep the original naming of these variables, so the registration function could potentially be used for other things? If nothing else, not all BrainGlobe atlases are of brains.
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 can change that back, at some point I considered passing a BrainGlobeAtlas object back but decided against it without changing it back.
src/bg_elastix/elastix/register.py
Outdated
) | ||
elastix_object.SetParameterObject(parameter_object) | ||
elastix_object.SetLogToConsole(log) | ||
elastix_object.LogToFileOn() | ||
elastix_object.SetOutputDirectory("./logs") |
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.
This fails for me because the directory doesn't exist by default. Also, should logging to file be optional (alongside saving the results? A typical napari plugin wouldn't log to file. If it needs to log, it should maybe go into ~.brainglobe
.
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 can remove that, it was mostly for my own early testing to see what I could glean from logs in terms of how well the registration went. I was hoping there would be a set of numbers in the logs that we could monitor to pick the "best" registration
src/bg_elastix/elastix/register.py
Outdated
) | ||
parameter_object.AddParameterMap(parameter_map_bspline) | ||
else: | ||
parameter_object.AddParameterFile( |
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 assume the plan is eventually for the user to be able to pick from a list of existing parameter files?
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.
That was my thought. We could curate some parameter sets from the model zoo, or other tools to present in a dropdown list of sorts.
src/bg_elastix/_widget.py
Outdated
] | ||
|
||
@register.get_atlas_button.changed.connect | ||
def get_atlas_button_click(): |
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.
It would be cool to use brainrender-napari
for this in some way rather than duplicating functionality here.
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've thought about this as well. I wasn't sure if we want to have a direct dependency. If we do, then I could reuse a lot of the QT code that @alessandrofelder has already written (such as the NapariAtlasRepresentation class). I also added a header to the plugin that's similar to brainrender-napari (has the brainglobe logo and links to repo/documentation). Is it worth potentially creating a brainglobe napari utils repository that all brainglobe napari plugins can depend on?
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.
Does it need to be a dependency? Is there a way of users just directly using the brainrender plugin to load the atlas?
Is it worth potentially creating a brainglobe napari utils repository that all brainglobe napari plugins can depend on?
Definitely. We'd previously agreed to do this, just haven't got round to it.
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.
Actually based on e.g. brainglobe/brainglobe-utils#3 I think we must have decided to put all napari stuff into https://github.com/brainglobe/brainglobe-utils
src/bg_elastix/_widget.py
Outdated
register.rotate.value = 0 | ||
|
||
@register.start_alignment_button.changed.connect | ||
def start_alignment_button_on_clik(): |
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.
def start_alignment_button_on_clik(): | |
def start_alignment_button_on_click(): |
src/bg_elastix/_widget.py
Outdated
register.translate_y.value, | ||
register.translate_x.value, | ||
) | ||
register.image_to_adjust.value.rotate = register.rotate.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.
This rotates at the origin, it may be more intuitive for the user to rotate at the center of the image? I don't think napari can handle this directly, but this plugin may be useful.
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 plugin looks great, easy to switch in
Looking very nice @IgorTatarnikov. I've left some comments. |
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.
Think this is a great start too 🎉 As discussed,
- maybe worth separating functionality from callbacks so same napari functionality usable from e.g IPython
- move from
magicgui
toqtpy
directly at some point
(This can/should probably happen in a future PR/future PRs?)
…_widget.py now uses qtpy directly to replicate the majority of _widget.py functionality.
for more information, see https://pre-commit.ci
I've added a new version in registration_widget.py that now uses qtpy directly. It's all in one file and unteste but the functionality is the same as the version in _widget.py. I'll work on writing unit tests/docs early next week and then switch to a more TDD approach now that I'm more familiar with qtpy and napari itself. |
…ual UI component into its own file
… default param settings
… the selection in the second column of the widget
…ameter maps can be run in one go
…just image button
…ed to allow comments in parameter file
…or brainglobe_registration/widgets/adjust_moving_image.py
I think this is ready as an early version. It can handle registering a 2D image to the selected atlas slice. |
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.
Looks good to me. I haven't gone through everything line by line, because I don't think it's very useful at this stage. I've raised issues for some larger points I had.
For some reason the tests fail for me with:
src/tests/test_adjust_moving_image_view.py Fatal Python error: Aborted
I wondered if I'm missing some dev dependencies?
…rlaid on sample image after run
Co-authored-by: Adam Tyson <[email protected]>
I think this has to do with #18. I've reformated the project in standardise-package-structure. I'll merge this PR and immediately open a fresh one to address the project restructuring, if that works for everyone. |
Go for it |
Before submitting a pull request (PR), please read the contributing guide.
Description
What is this PR
Why is this PR needed?
Starting development on bg-elastix. Allow ideal 2D images to be registered to an atlas using elastix.
What does this PR do?
References
#12
How has this PR been tested?
No tests currently written, but all code should have unit tests at least
Is this a breaking change?
N/A
Does this PR require an update to the documentation?
Yes
If any features have changed, or have been added. Please explain how the documentation has been updated (and link to the associated PR). See here for details.
Checklist: