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
Fixed problem with parameters xmax, ymax and zmax #2604
Conversation
spinalcordtoolbox/cropping.py
Outdated
@@ -38,9 +38,9 @@ def _get_max_value(input, dim): | |||
# If empty, return dim+1 (corresponds to the maximum of the given dimension, e.g. nx) | |||
if input is None: | |||
return dim + 1 |
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.
to be consistent, this should be dim (not dim+1), right?
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.
Yes, you're right! Thx
spinalcordtoolbox/cropping.py
Outdated
@@ -38,9 +38,9 @@ def _get_max_value(input, dim): | |||
# If empty, return dim+1 (corresponds to the maximum of the given dimension, e.g. nx) | |||
if input is None: | |||
return dim + 1 | |||
# If negative sign, return dim+1 if -1, dim if -2, dim-1 if -3, etc. | |||
# If negative sign, return dim if -1, dim-1 if -2, dim-2 if -3, etc. |
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.
To clarify (the comment is in contradiction with the code), i would write:
# If input is "-1", return maximum dimension (i.e. no change). If input is "-2", returns maximum dimension minus one, etc.
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.
Thanks @AnBucquet-- i would also clarify the usage:
-xmin <int> Lower bound for cropping along X.
-xmax <int> Higher bound for cropping along X. Setting '-1' will crop to the maximum dimension (i.e. no change), '-2' will crop to the maximum dimension minus 1 slice, etc.
-ymin <int> Lower bound for cropping along Y.
-ymax <int> Higher bound for cropping along Y. Follows the same rules as xmax.
-zmin <int> Lower bound for cropping along Z. Follows the same rules as xmax.
-zmax <int> Higher bound for cropping along Z. Follows the same rules as xmax.
It should be good now. Thx for the review! Let me know if you find anything else. |
Need to change |
help="Higher bound for cropping along X. Setting '-1' will crop to the maximum dimension, '-2' will crop to " | ||
"the maximum dimension minus 1 slice, etc.", | ||
help="Higher bound for cropping along X. Setting '-1' will crop to the maximum dimension (i.e. no change), " | ||
"'-2' will crop to the maximum dimension minus 1 slice, etc.", |
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.
zmin should be: "Lower bound for cropping along Z.",
zmax should be: "Higher bound for cropping along Z. Follows the same rules as xmax."
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 just removed the
+1
in that line to get the correct dimension.Fixes #2603