Rewrite -add/-sub/-mul/-div to match expected behavior for 3D/4D images
#3808
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
There's no need to call either `get_data_or_scalar` or `concatenate_along_4th`, since all we're really doing in sct_create_mask is setting the data array to zero. The reason I'm doing this is that I'm going to be making changes to the sct_maths functions in future commits. So, it saves some headache to decouple sct_maths from sct_create_mask.
This will allow us to more easily refactor the `concatenate` function in the next commit.
This results in the following behavior change:
- Before: 3D volumes within a 4D image are summed ("[sum(4D) ==> 3D] + [sum(4D) ==> 3D] = 3D")
- After: 4D images are summed directly ("4D + 4D = 4D")
This results in `-sub` and `-div` now being able to accept lists of images.
This allows us to add/mul 3D/4D volumes together without throwing an error.
joshuacwnewton
added
bug
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more.
|
joshuacwnewton
commented
Jun 14, 2022
mguaypaq
requested changes
Jun 17, 2022
There was a problem hiding this comment.
Thanks for the new feature and the incidental cleanup! I don't want to increase the scope of this PR too much, but I do think that get_data_or_scalar and get_data should be made less convoluted. Also, the zero-argument version of -add should be copied over to -mul too (but not -sub or -div).
Sorry for the many comments; they reflect the state of the existing code more than your changes (which are already definite improvements).
Feel free to ping me on Slack if you'd like a more interactive review and/or brainstorming for solutions.
If the files don't exist, then we can just rely on the error thrown by `Image()`. As written, the try/except block didn't work as expected.
Dimension-checking is only relevant for "Case 2", since Case 1 guarantees matching input shapes. Also, we don't need to check all the images against each other; we only need to check against the input image.
This disallows combining 3D/4D images, so we remove those now-failing test cases.
This makes it clearer that `get_data_or_scalar` is specifically designed to parse specific arguments in sct_maths
Allows the 3D volumes within a 4D image to be multiplied together.
…tions By removing this message, it makes the help description a little less verbose/confusing. Since, "dimensions must match" doesn't make any sense for scalar arguments or bare -add/-mul argument So, I think it might be better to assume that the user will pass images with matching dimensions. And, if they don't, they will find out the correct usage via the error message in the parsing action.
I appreciate this nudge! I agree that the functions are unnecessarily complex, and much could be done to make them simpler. I've tried rewriting the function into a parsing action that incorporates your suggestions. Let me know what you think! |
mguaypaq
approved these changes
Jun 22, 2022
Co-authored-by: Mathieu Guay-Paquet <mathieu.guaypaquet@gmail.com>
Co-authored-by: Mathieu Guay-Paquet <mathieu.guaypaquet@gmail.com>
Ahh, good thinking! Since we're using On the one hand, it's not the prettiest? But on the other hand, it's more or less identical to every other traceback we get when loading image files. |
Before, we were parametrizing add/mul differently than sub/div, so it made sense to have different tests. But, now we use the same set of input image dimensions, so we no longer need to separate the tests.
NB: The parser should catch the error before the Python API ever does, so the error handling code in the API doesn't ever get tested. But, maybe this is fine?
joshuacwnewton
force-pushed
the
jn/3806-fix_sct_maths_add_4d_data
branch
from
702a278
to
b5ee93d
Compare
|
I did some additional clean-up, since I realized there were some stray details that needed to be fixed after we removed support for mixed 3D/4D images in -add/-mul. One last tiny review for the new changes and this should be good to go? |
mguaypaq
approved these changes
Jun 27, 2022
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.
Checklist
GitHub
PR contents
Description
This PR adds new tests to check the dimensions of the output for
-add/-sub/-mul/-div. On master, this test fails for:-add/-mulwith 4D images-sub/-divwith lists of imagesSo, this PR also adds some rewriting to simplify the underlying functions (
get_data_or_scalar(),concatenate_along_4th_dimension()) to make the tests pass.Finally, this PR improves the documentation of the functions to make the behaviors clearer (#3806 (comment), #3806 (comment)).
Linked issues
Fixes #3806.