Use argument-list subprocess calls for NiftyReg commands

[AlgoFoe]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?

• NiftyReg commands in brainreg are currently constructed as shell strings, which can be fragile when file paths contain spaces and lead to platform-dependent behavior.

What does this PR do?

• It updates all NiftyReg command invocations to use argument-list based subprocess execution instead of shell-based string commands. This removes the need for manual quoting and safely handles paths with spaces on all platforms.

References

• brainreg issue [BUG] NiftyReg doesn't deal with spaces in file names #4
• depends on corresponding update in brainglobe-utils #144

How has this PR been tested?

• Tested locally on Windows with input, output, and install paths containing spaces.

Is this a breaking change?

• No.

Does this PR require an update to the documentation?

• No.

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality (unit & integration)
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[AlgoFoe]

I think the CI failure may be due to NiftyReg argument ordering. Moving to list-based execution seems to expose that -ref / -flo need to appear before other flags for reg_aladin and reg_f3d.
I have a possible fix that reorders the command construction, happy to push it if that makes sense.

[AlgoFoe]

I think the CI failure may be due to NiftyReg argument ordering. Moving to list-based execution seems to expose that -ref / -flo need to appear before other flags for reg_aladin and reg_f3d. I have a possible fix that reorders the command construction, happy to push it if that makes sense.

Reference Image:

I had a closer look at reg_aladin.cpp, and it looks like -ref and -flo are expected early in the argument list.

[IgorTatarnikov]

I have a feeling this error will be resolved when we use the new version of brainglobe-utils. Based on my testing, if subprocess.check_call is passed a list as the cmd and shell=True (the current behaviour of brainglobe-utils) only the first element of the list is passed to the command line (i.e. it's as if it's only calling the binary with no other arguments).

[AlgoFoe]

Got it, thanks for clarifying! I’ll hold off on changing the argument order and let this resolve once brainreg is using the updated brainglobe-utils

[IgorTatarnikov]

Would it be possible to add a test in tests/tests/test_integration/test_registration.py that uses an output path with a space in it. There would be no need to compare the output to the regression data, just ensuring that the files are created.

[AlgoFoe]

Would it be possible to add a test in tests/tests/test_integration/test_registration.py that uses an output path with a space in it. There would be no need to compare the output to the regression data, just ensuring that the files are created.

Yes, that makes sense. I will update _prepare_openmp_thread_flag to store the flag as a list and also add a test.
I will push the update shortly.

[AlgoFoe]

Would it be possible to add a test in tests/tests/test_integration/test_registration.py that uses an output path with a space in it. There would be no need to compare the output to the regression data, just ensuring that the files are created.

Yes, that makes sense. I will update _prepare_openmp_thread_flag to store the flag as a list and also add a test. I will push the update shortly.

I have pushed the update and also added a missing SegmentationError raise.

[AlgoFoe]

I think codecov seems to be failing because the new raise SegmentationError line isn’t covered by tests. I can add a small test to cover this if that’s okay.

[IgorTatarnikov]

That would be great!

[AlgoFoe]

That would be great!

Added the missing SegmentationError tests and pushed the update.

[IgorTatarnikov]

Thank you, this is almost ready to go! Could you pin brainglobe-utils >= 0.10.0 in pyproject.toml.

[IgorTatarnikov]

LGTM 🚀

Thank you @AlgoFoe