Update voxel / world method #28
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 440 503 +63
=========================================
+ Hits 440 503 +63
|
|
Starting to review this but needs a merge from main |
tkkuehn
approved these changes
Aug 31, 2023
| world_to_voxel(voxel_coord, nii_affine) | ||
|
|
||
|
|
||
| class TestAfidVoxel2World: |
There was a problem hiding this comment.
It would be nice to add a round-trip test that confirms that going back and forth yields a value close to the original (I think numpy has some method for "floats approximately equal"
There was a problem hiding this comment.
Added some tests for this, though really had to loosen the approximation for the world_to_voxel round trip tests to pass. I think this is a combination of the rounding errors and also a lack of constraints on the affine matrix.
- Also rename strategies to following SearchStrategy convention
kaitj
force-pushed
the
maint/world_voxel
branch
from
f29da5d
to
1aa990a
Compare
- renamed valid_coords() -> valid_position_coords() - added strategy for generating valid_voxel_coords() - constrained coords to be about 90mm (in either direction) or 200 voxels - added strategy for generating afid_voxels - removed obsolete world_coords strategy (replaced by afid_positions)
|
Note to self to check transformation methods (can refer to nibabel docs) |
Note: `world_to_voxel` round trip required very loose approximation, due to lack of constraints in generated transformation and rounding errors
kaitj
force-pushed
the
maint/world_voxel
branch
from
fdbd218
to
9005dc3
Compare
tkkuehn
approved these changes
Sep 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
This updates the methods for transforming from world-to-voxel coordinates and also adds in method for voxel-to-world coordinates. Also updates to return either an AfidPosition or AfidVoxel as needed instead of a numpy array of values. This still retains using of NumPy, which is helpful for manipulating matrices.
I debated adding this as a method of either the AfidPosition or AfidVoxel class, but opted against it for now. Open for discussion on this though.
Types of changes
What types of changes does your code introduce? Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!poe qualitytaskNotes
All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.