Skip to content

mrcat: Change positional argument structure#2792

Merged
Lestropie merged 3 commits intodevfrom
mrcat_interface
Mar 4, 2024
Merged

mrcat: Change positional argument structure#2792
Lestropie merged 3 commits intodevfrom
mrcat_interface

Conversation

@Lestropie
Copy link
Member

Currently, mrcat enforces at the point of command-line parsing that there are at least two input images. It does this by having Argument image, then Argument image2 with .allow_multiple().

I have two issues with this design:

  1. Consider Implement Pydra code-generators #2665. Ideally, the utilisation of mrcat in a Pydra workflow would look something like:

        images = ['image1.mif', 'image2.mif']
        workflow.add(mrcat(inputs=images, ...))
    

    However with an automatically-generated Pydra interface, what would actually be required
    would end up being a rather clunky:

        workflow.add(mrcat(image1=images[0], image2=images[1:], ...))
    
  2. This mechanism is absent from other commands where it is equally applicable; eg. mraverageheader, transformcompose. So the software is not internally consistent in this regard.

For the first point in particular, I think it makes sense to have a single positional input argument with .allow_multiple(). But I am open to a counter-argument.


Hypothetically, we could introduce some other Argument modifier that would set the minimum number of occurrences. This would allow catching command mis-use at the point of command-line parsing. But we would then need to consider how to reflect this information in the various __print_usage__ interface exports. Any thoughts?

No longer enforce the presence of at least two input images during command-line parsing; instead defer this to the run() function.
@Lestropie Lestropie self-assigned this Jan 31, 2024
@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member Author

After further consideration in the context of Pydra workflow construction, it's possible that changing the mrcat operation with a single input from an Exception to a WARN() may be preferable.

Copy link
Member

@jdtournier jdtournier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully agree with these changes. Not sure why we'd implemented it that way in the first place, in hindsight...

@jdtournier
Copy link
Member

After further consideration in the context of Pydra workflow construction, it's possible that changing the mrcat operation with a single input from an Exception to a WARN() may be preferable.

Personally, I don't think this should even raise a warning. There's nothing theoretically wrong with passing a single input image to mrcat, and I can see situations where some script may perfectly legitimately need to concatenate one or more inputs, and raising a warning every time the user happens to provide a single input would just cause needless confusion. Just my 2¢...

@Lestropie Lestropie enabled auto-merge March 4, 2024 23:03
@github-actions
Copy link

github-actions bot commented Mar 4, 2024

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie Lestropie merged commit 5cd596a into dev Mar 4, 2024
@Lestropie Lestropie deleted the mrcat_interface branch March 4, 2024 23:34
@Lestropie Lestropie restored the mrcat_interface branch August 26, 2025 07:51
@Lestropie Lestropie deleted the mrcat_interface branch August 27, 2025 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants