TEST: Cover the doctest, the ROI assertion and slice_affine directly - #1
Open
Arthur031221 wants to merge 1 commit into
Open
Conversation
Three things the fix needs alongside it. The flip example in doc/source/nibabel_images.rst prints an X translation of 117.86, which is the value from before the fix. With the fix in place that doctest fails, and the job that would catch it is continue-on-error, so it would land quietly. test_nib_roi asserted np.allclose(in_sliced.affine, out_img.affine), which compares the slicer against itself and passes whatever the slicer does. It already exercises -j -1:1:-1, so that assertion is why this was never caught there. It now compares both against an affine calculated independently. test_slicer_preserves_ras_position went through img.slicer only. One line asserts slice_affine directly as well, so the affine calculator is covered on its own rather than through its caller.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The three pieces from nipy#1533 that aren't in this branch yet. Thanks for the offer.
doc/source/nibabel_images.rstprints an X translation of117.86for the flipped image, which is the pre-fix value. With your change in place that doctest fails:With this it's 71 passed, 0 failed. Worth catching now because the doctest job is
continue-on-error: true, so it would land without anything going red.test_nib_roiassertednp.allclose(in_sliced.affine, out_img.affine). That compares the slicer against itself, so it passes whatever the slicer does. It already exercises-j -1:1:-1, which is why this bug was never caught there. Comparing against an independently calculated affine instead: with your fix reverted the old assertion still gives 12 passed, the new one gives 2 failed.One line in
test_slicer_preserves_ras_positionassertsslice_affinedirectly rather than only throughimg.slicer, since effigies asked for the affine calculator and the slicer separately. Reverting the fix takes it from 10 passed to 10 failed.I put it inside your existing loop rather than adding a parallel test, so the same nine slicers cover both. Say the word if you would rather have it separate, or want any of this dropped.