Save output with the same format as brainreg
#66
Conversation
…tations without errors
…mation parameters for elastix
IgorTatarnikov
changed the title
brainregbrainreg
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
==========================================
- Coverage 82.09% 81.02% -1.07%
==========================================
Files 9 9
Lines 592 738 +146
==========================================
+ Hits 486 598 +112
- Misses 106 140 +34 ☔ View full report in Codecov by Sentry. |
|
Looks nice, is the plan for this to allow compatibility with other tools that expect a brainreg output? If so, I think the next step should be to add a loader for brainglobe-napari-io, and then look into adding suport in brainglobe-segmentation and brainmapper. |
|
That's the plan, I'll look into adding a loader for |
There was a problem hiding this comment.
Very nice. A couple of suggestions/thoughts:
- The output directory should have some kind of specific identifier that it came from
brainglobe-registration. If we’re modelling the output on brainreg, it should be abrainglobe-registration.jsonthat stores registration metadata. This will then “tell”brainglobe-napari-iothat it can load the directory, and how it should load it. - I’m not sure about the workflow where the user specifies an output directory if they want the results to be saved. I can see a likely scenario in which the user runs registration (which may take a while), but they don’t save it. They may expect to be able to save it after running, but they cannot. I think it should be made more obvious how to save the outputs (it’s currently one part of a very large widget), or it should be possible to save the output after registration. I think this can wait for a new PR though.
| for i in range(len(parameter_lists)) | ||
| ] | ||
|
|
||
| itk.ParameterObject.WriteParameterFile( |
There was a problem hiding this comment.
Does it make sense to use pathlib here?
There was a problem hiding this comment.
I tried using pathlib but ITK wants a string, and crashes otherwise. It felt a bit convoluted to create a bunch of Path objects just to convert them back to strings before using them.
| transformix_object.GetOutputDeformationField() | ||
| )[..., ::-1] | ||
|
|
||
| # Cleanup files generated by elastix |
There was a problem hiding this comment.
Is there any conceivable reason to keep these files, e.g. in a "Debug" mode that could be chosen by the user?
There was a problem hiding this comment.
I'm not sure, the generated file is the deformation field saved in (x, y, z) rather than (z, y, x). I'll add a debug flag to the Python API but won't expose it via the widget at this time.
|
Agreed on both points. Important configurations are now saved in a |
Before submitting a pull request (PR), please read the contributing guide.
Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)
Description
What is this PR
Why is this PR needed?
Currently, the output of
brainglobe-registrationare displayed in thenapariviewer but not saved anywhere.What does this PR do?
This PR adds functionality to save the outputs to a specified
output_directory. The forward and backwardelastixparameters are saved, along with files equivalent tobrainreg.Refactored
elastix/register.py.Added a function to remap values greater than 216 to consecutive values starting at 216. This prevents floating point errors when transforming using
transformixas ITK only supports float32 and not float64 in the pypi wheels. The changes are reverted following transformation.Added expected outputs that can be used for regression testing.
References
#63
How has this PR been tested?
Added tests for new functionality.
Is this a breaking change?
No
Does this PR require an update to the documentation?
Yes, will come later.
Checklist: