nipreps / eddymotion

Open-source eddy-current and head-motion correction for dMRI.
https://nipreps.org/eddymotion
Apache License 2.0
13 stars 16 forks source link

ENH: Add CLI entry point test #184

Closed jhlegarreta closed 3 months ago

jhlegarreta commented 5 months ago

Add CLI entry point test.

jhlegarreta commented 5 months ago

It is unclear to me the proper way to test this.

Have looked in other nipreps repositories and found different ways to test main: nondefaced-detector: https://github.com/nipreps/nondefaced-detector/blob/abf20d244de51a9a73933cbf3cf64fad163f6fa0/nondefaced_detector/cli/tests/main_test.py#L10C25-L11C50 sdcflows: https://github.com/nipreps/sdcflows/blob/357ef3e683f05a1722c113b1dc94ceed2ed02a53/sdcflows/cli/tests/test_find_estimators.py#L144

Do we need to add an argv=None to main ? e.g. https://github.com/nipreps/sdcflows/blob/357ef3e683f05a1722c113b1dc94ceed2ed02a53/sdcflows/cli/main.py#L26 https://github.com/nipreps/migas-server/blob/24536bbd46423a94d3dae29702d32af4435f3ed7/migas/server/cli.py#L30 https://github.com/nipreps/nibabies/blob/7b1e91d5d84094129caa29e16803f842605eaa23/nibabies/cli/mcribs.py#L24

Comments are welcome.

esavary commented 5 months ago

It is unclear to me the proper way to test this.

Have looked in other nipreps repositories and found different ways to test main: nondefaced-detector: https://github.com/nipreps/nondefaced-detector/blob/abf20d244de51a9a73933cbf3cf64fad163f6fa0/nondefaced_detector/cli/tests/main_test.py#L10C25-L11C50 sdcflows: https://github.com/nipreps/sdcflows/blob/357ef3e683f05a1722c113b1dc94ceed2ed02a53/sdcflows/cli/tests/test_find_estimators.py#L144

Do we need to add an argv=None to main ? e.g. https://github.com/nipreps/sdcflows/blob/357ef3e683f05a1722c113b1dc94ceed2ed02a53/sdcflows/cli/main.py#L26 https://github.com/nipreps/migas-server/blob/24536bbd46423a94d3dae29702d32af4435f3ed7/migas/server/cli.py#L30 https://github.com/nipreps/nibabies/blob/7b1e91d5d84094129caa29e16803f842605eaa23/nibabies/cli/mcribs.py#L24

Comments are welcome.

Passing argv=None to the main function seems like the best choice to me. @effigies @oesteban , do either of you have strong opinions on this?

jhlegarreta commented 4 months ago

Sorry to ping you again this morning @effigies @oesteban.

jhlegarreta commented 4 months ago

@effigies Maybe you have more insight into https://github.com/nipreps/eddymotion/pull/184#issuecomment-2080813005. Suggestions are welcome. Thanks.

oesteban commented 3 months ago

@effigies, can you have a look if you meet some bandwidth?

effigies commented 3 months ago

Sure, had a look and just went ahead and patched. I think the main(argv=None) pattern is simple enough. I moved the monkeypatching just to set the command to eddymotion for realism (otherwise checking output would show usage: pytest [-h] ...).

Ran into an empty array error here:

src/eddymotion/cli/run.py:47: in main
    _ = estimator.estimate(
src/eddymotion/estimator.py:170: in estimate
    fixed, moving = _prepare_registration_data(
src/eddymotion/estimator.py:336: in _prepare_registration_data
    _to_nifti(dwframe, affine, moving)
src/eddymotion/estimator.py:243: in _to_nifti
    data = _advanced_clip(data)
src/eddymotion/estimator.py:223: in _advanced_clip
    a_min = np.percentile(denoised[denoised > 0] if nonnegative else denoised, p_min)

Simple fix to change that to >=0, which matches the variable.