Allow multiple sct_maths operations to be run sequentially on an image in one command
#4485
Conversation
This small refactor will make repeatedly applying array operations a lot easier!
joshuacwnewton
added
enhancement
joshuacwnewton
force-pushed
the
jn/1447-sct_maths-sequential-operations
branch
from
0e5060d
to
9cafa7f
Compare
There was a problem hiding this comment.
This is a nice attempt: it tries to make minimal changes to the existing code, and it respects the order of operations on the command line.
But, I'm not a fan of this design, because it loses the connection between the operations and their arguments. For example, when I ran the following test, I got the wrong result:
sct_maths -i t2.nii -add 1 -add 2 -o result.nii
The problem is that arguments.add only stores the last value, so both operations added 2, instead of 1 and 2.
I think we'll need the argument parser to actually build a list of operations (and their arguments). But we should carefully think about the possible interactions between arguments, because this change turns the sct_maths command-line into a mini programming language. In particular, the current "mathematical morphology" options -shape and -dim sort of work like global variables, which is tricky. And the "similarity metric" options don't respect the "image in -> image out" function signature.
|
Ahhh, very true -- relying on the default behavior of argparse (via Truthfully, though, I'm still a bit hesitant about using a custom action to implement this behavior? I feel like the complexity increases quite a bit when we try to account for behavior beyond the simplest cases. But, I think I have an idea of how to resolve the conflict between a new custom action and the existing custom action! EDIT: I've implemented my idea in 805be65. |
My main concern was needing to specify a new `CustomAction` for every single argument. That felt like
a lot of unnecessary boilerplate to me? And, I was concerned about the overlap between this new
custom action and the existing `ParseDataOrScalarArgument` custom action.
I solved these issues by:
- Creating a `store` function that can be re-used by the custom actions.
- Instead of creating an entirely new CustomAction, I just subclass the built-in `_StoreAction`, then
register this new StoreAction as the action for both 'action=None' and 'action="store"'.
(This is what is done internally in argparse, so I don't think it's too "internal". despite us
accessing the internal _StoreAction class.)
See: https://github.com/python/cpython/blob/d472b4f9fa4fb6061588d421f33a0388a2005bc6/Lib/argparse.py#L1325-L1327
I like this method because it only modifies the `parser` object used by `sct_maths`.
Rather than accessing the parsed argument values from `arguments` (which will be overwritten when the same argument is passed multiple times), we fetch the argument values from the new `arguments.ordered_args`, which preserves the arguments (and their parsed values) in the order they were passed. One thing I don't love about this solution is that we still need to access the "global variables" `arguments.shape` and `arguments.dim`, which was predicted by @mguaypaq in #4485 (review). I'll try to puzzle out a better way of doing this...
This avoids us having to pass `arguments` to `apply_array_operation`, but I feel like I'd much rather remove `-shape` and `-dim` entirely, and instead have these values passed directly to `-dilate` and `-erode`. This would allow us to specify multiple different values for shape/dim, one per call to `-dilate`/`-erode`. As it stands, these values are "global", which is a bit of a hindrance for the user.
joshuacwnewton
force-pushed
the
jn/1447-sct_maths-sequential-operations
branch
from
0de6667
to
dd2656e
Compare
Description
This PR allows users to provide multiple arguments and have them be run sequentially, in the order provided. Example:
This PR adds a test, then updates
sct_mathsto iterate over the input arguments, running one operation at a time.I decided to go with the "parse
argvto get argument order" method as opposed to the "add a custom action to collect the arguments" method because I figured it was the most straightforward, and it allows us to use other actions for our argparse options.Given that this is a large refactor, this probably needs some thorough testing for edge cases, but I wanted to get a PR open prior to the meeting.
Linked issues
Fixes #1447.