TST: Add back CLI main entry point testing #106

jhlegarreta · 2025-05-03T18:16:43Z

FIX: Fix output file in NiFreeze entry point function
FIX: Make the generated random brain mask be of boolean type
REF: Refactor the parser module
STY: Prefer argv as the name of the arguments of the parser

Fixes #101.

codecov · 2025-05-03T18:20:23Z

Codecov Report

Attention: Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.15%. Comparing base (fb8f3f3) to head (258764c).

Files with missing lines	Patch %	Lines
src/nifreeze/cli/parser.py	93.54%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   71.35%   74.15%   +2.80%     
==========================================
  Files          23       23              
  Lines        1138     1153      +15     
  Branches      139      140       +1     
==========================================
+ Hits          812      855      +43     
+ Misses        283      249      -34     
- Partials       43       49       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jhlegarreta · 2025-05-03T18:49:51Z

@oesteban Not sure whether you want to add more testing to this now or whether that can be left for future work.

oesteban · 2025-05-03T19:09:30Z

I moved that test to the parser (it felt more appropriate):

nifreeze/test/test_parser.py

Lines 103 to 127 in e488df2

    
           def test_parser(tmp_path, datadir): 
        
               input_file = datadir / "dwi.h5" 
        
               with pytest.raises(SystemExit): 
        
                   build_parser().parse_args([str(input_file), str(tmp_path)]) 
        
               args = build_parser().parse_args( 
        
                   [ 
        
                       str(input_file), 
        
                       "--models", 
        
                       "trivial", 
        
                       "--nthreads", 
        
                       "1", 
        
                       "--njobs", 
        
                       "1", 
        
                       "--seed", 
        
                       "1234", 
        
                       "--output-dir", 
        
                       str(tmp_path), 
        
                   ] 
        
               ) 
        
               assert args.input_file == input_file 
        
               assert args.njobs == 1 
        
               assert args.output_dir == tmp_path

Rather than testing the CLI, what's missing is separating the parsing from the "main" function. That way, we will be able to offer that is usable to other libraries, as opposed to the current implementation that includes the parsing in the begining of the function.

jhlegarreta · 2025-05-08T02:27:03Z

I see. My bad.

And we do not want to keep the following?
https://github.com/nipreps/nifreeze/pull/106/files#diff-959a1effd1cb0e25016584deef9be1994af65814f318346bf3a63801b5db1636R32-R36

Re nipreps/eddymotion#184.

Rather than testing the CLI, what's missing is separating the parsing from the "main" function. That way, we will be able to offer that is usable to other libraries, as opposed to the current implementation that includes the parsing in the begining of the function.

So you mean changing

nifreeze/src/nifreeze/cli/run.py

Lines 32 to 42 in e488df2

    
           def main(argv=None) -> None: 
        
               """ 
        
               Entry point. 
        
               Returns 
        
               ------- 
        
               None 
        
               """ 
        
               parser = build_parser() 
        
               args = parser.parse_args(argv)

to?

def main(args) -> None: 
    (...)


if __name__ == "__main__":
    parser = build_parser()
    args = parser.parse_args(argv) 
    main(args)

I guess we also want to test running the CLI with actual data.

jhlegarreta · 2025-05-31T19:48:40Z

Pinging @oesteban re #106 (comment).

jhlegarreta · 2025-06-14T18:07:59Z

src/nifreeze/cli/run.py

-        dataset.to_filename(output_path)
+        output_h5_filename = Path(Path(args.input_file).name).stem + ".h5"
+        output_h5_path: Path = Path(args.output_dir) / output_h5_filename
+        dataset.to_filename(output_h5_path)


Although having an HDF5 file as input seems the best way for testing,

nifreeze/src/nifreeze/cli/parser.py

Line 68 in 94f6d27

help="Path to the HDF5 file containing the original 4D data.",

In the mid-term we will want to accept NIfTI files. So the above will need to change. This will need to be added to testing. The above is especially apparent in
https://github.com/nipreps/nifreeze/pull/106/files#diff-240d2e4cff5653f9b0b21aaf6d06a74165c1b72f21c0717735d5c26b41bd52c6R178

where the test loads an HDF5 file that has already gradient information, but the test reads the same/other gradient data files for the sake of testing.

To be addressed in a separate PR.

jhlegarreta · 2025-06-14T18:11:58Z

test/test_main.py

+    # Define smoke run method
+    def smoke_estimator_run(self, dataset, **kwargs):
+        called["dataset"] = dataset
+        called["kwargs"] = kwargs


I do not know if a better/more pythonic way of monkeypatching can be proposed. I'd be glad to have some suggestions.

jhlegarreta · 2025-06-14T19:58:53Z

src/nifreeze/cli/run.py

        estimator: Estimator = Estimator(
-            _model.lower().replace("single", ""),
+            _model,
            prev=prev_model,
            single_fit=single_fit,
            model_kwargs=model_kwargs,


The estimator accepts other keyword arguments:

nifreeze/src/nifreeze/estimator.py

Line 84 in 94f6d27

**kwargs,

To be fixed in a separate PR.

jhlegarreta · 2025-06-14T19:59:49Z

The above comments (except for the monkeypatching question) are left for the records and for separate/future PRs. Would be grateful if this could be reviewed and potentially merged soon unless I got it wrong.

jhlegarreta · 2025-06-14T21:35:10Z

test/test_parser.py

+    "models",
+    [
+        "dti",
+        "dki",


Did not include averagedwi:

nifreeze/src/nifreeze/model/base.py

Line 61 in 7cfc148

if model.lower() in ("avg", "average", "mean"):

because I am testing here the _model_class property:
https://github.com/nipreps/nifreeze/pull/106/files#diff-240d2e4cff5653f9b0b21aaf6d06a74165c1b72f21c0717735d5c26b41bd52c6R251

which AverageDWIModel does not have.

The TrivialModel and ExpectationModel (to be done in a separate function) were not added due to the same reason.

Maybe some other property could be tested and this function be refactored. To be done in a separate PR.

Slightly related, even if an S0 model is mentioned in

nifreeze/src/nifreeze/model/base.py

Line 47 in 7cfc148

Options: ``"DTI"``, ``"DKI"``, ``"S0"``, ``"AverageDWI"``

, it is not implemented or its alias not included elsewhere.

We'll need to add PET as well eventually. This list should be defined at a single place, IMO.

oesteban

A few nit picks

src/nifreeze/cli/parser.py

test/conftest.py

test/data/hcph_multishell.txt

Fix output file in `NiFreeze` entry point function: the input file is expected to be an HDF5 currently, but the serialization of the output is done through the `to_nifti` method, which serializes to `NIfTI` format. Since the extension of the input file is not changed, the method fails to serialized the output data. Fixes: ``` E nibabel.filename_parser.TypesFilenamesError: File extension ".h5" was not in expected list: ['.nii'] ``` raised while adding CLI main entry point testing. Add CLI main entry point testing: the original contents were transferred to `test_parser.py` across commits 0aaacda, b52f052 and f9bbcc6 as they belonged to the `parser` module. The CLI main entry point has been lacking proper testing since the origin. This patch set: - Reintroduces the `test_main.py` test file to test the CLI main entry point exclusively, leaving testing the parser to `test_parser.py`. - Moves the `test_help` function to `test_main.py` as it strictly tests the main call, not the parser logic. - Adds a smoke test to check that the main function is called and output files are written as evidence that the body of the function is executed correctly without actually running the estimation process (it is monkey patched in the test). - Fixes the aforementioned bug.

Make the generated random brain mask be of boolean type. Convert it to unsigned integer type for practical purposes, as `nibabel` does not support boolean types when serializing data. Fixes: ``` self._S0 = np.full( self._data_mask.sum(), > np.round(np.percentile(dataset.dataobj[self._data_mask, ...], 98)), ) E IndexError: arrays used as indices must be of integer (or boolean) type src/nifreeze/model/dmri.py:94: IndexError ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/15654840868/job/44104507991?pr=106#step:11:712 Inadvertently introduced in commit 63e6f37.

Refactor the parser module: - Refactor the parsing logic from the CLI main entry point to a new `parse_args` function. In doing so, `build_parser` becomes a function that is intended to stay private in the `parser` module and the only function that is called from `main` is `parse_args`. This allows to confine the complexity of the parsing logic in the `parser` module, and makes the main function more lightweight, and less susceptible to changes derived from code refactoring. - Return an extra keyword argument parameter in `parse_args` in anticipation of forwarding them to the estimator. - Rename the `test_parser` function to `test_parser_extended`, parameterize it, and check that all input arguments are correctly processed to the destination names. - Add the necessary tests to check the parser module. Most notably, add tests to ensure that single fit models are proccessed correctly, that DWI data files are processed correctly, and that DWI models and the estimator can be instantiated correctly with the information provided by the parser (as an evidence that they can be instantiated in the main function following that rationale). Add the `hcph_multishell.txt` gradient file to the testing data suite so that it can be used to test the use of gradient data formatted in such way. The file was created reading the existing `hcph_multishell.bval` and `hcph_multishell.bvec` files as text files and simply stacking them vertically, with a maximum of 6 decimal places.

Prefer `argv` over `args` as the name of the arguments of the parser, and use `args` to name the values returned by the parser. Increases consistency with what the naming used by the argument parsing function and the rest of the test functions.

jhlegarreta · 2025-07-09T14:35:14Z

Ready to be merged. Improvements can come in separate PRs.

jhlegarreta force-pushed the AddBackCLIMainEntryPointTesting branch 2 times, most recently from 9365f22 to 55fd2cd Compare May 3, 2025 18:37

jhlegarreta mentioned this pull request Jun 9, 2025

Add back testing the CLI main entry point #101

Open

jhlegarreta force-pushed the AddBackCLIMainEntryPointTesting branch 4 times, most recently from b052875 to 9def891 Compare June 14, 2025 18:00

jhlegarreta commented Jun 14, 2025

View reviewed changes

jhlegarreta force-pushed the AddBackCLIMainEntryPointTesting branch 3 times, most recently from 3826b90 to ff2635d Compare June 14, 2025 19:46

jhlegarreta commented Jun 14, 2025

View reviewed changes

jhlegarreta marked this pull request as ready for review June 14, 2025 20:27

jhlegarreta force-pushed the AddBackCLIMainEntryPointTesting branch from ff2635d to 50a02ac Compare June 14, 2025 20:49

jhlegarreta commented Jun 14, 2025

View reviewed changes

oesteban reviewed Jul 8, 2025

View reviewed changes

src/nifreeze/cli/parser.py Outdated Show resolved Hide resolved

test/conftest.py Show resolved Hide resolved

test/data/hcph_multishell.txt Show resolved Hide resolved

jhlegarreta added 2 commits July 8, 2025 10:05

jhlegarreta force-pushed the AddBackCLIMainEntryPointTesting branch 2 times, most recently from cdd6374 to e828e3f Compare July 8, 2025 14:27

jhlegarreta added 2 commits July 9, 2025 10:21

jhlegarreta force-pushed the AddBackCLIMainEntryPointTesting branch from e828e3f to 258764c Compare July 9, 2025 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TST: Add back CLI main entry point testing #106

TST: Add back CLI main entry point testing #106

Uh oh!

jhlegarreta commented May 3, 2025 •

edited

Loading

Uh oh!

codecov bot commented May 3, 2025 •

edited

Loading

Uh oh!

jhlegarreta commented May 3, 2025

Uh oh!

oesteban commented May 3, 2025

Uh oh!

jhlegarreta commented May 8, 2025 •

edited

Loading

Uh oh!

jhlegarreta commented May 31, 2025

Uh oh!

jhlegarreta Jun 14, 2025 •

edited

Loading

Uh oh!

jhlegarreta Jun 14, 2025

Uh oh!

jhlegarreta Jun 14, 2025

Uh oh!

jhlegarreta commented Jun 14, 2025

Uh oh!

jhlegarreta Jun 14, 2025 •

edited

Loading

Uh oh!

oesteban left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhlegarreta commented Jul 9, 2025

Uh oh!

Uh oh!

TST: Add back CLI main entry point testing #106

Are you sure you want to change the base?

TST: Add back CLI main entry point testing #106

Uh oh!

Conversation

jhlegarreta commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jhlegarreta commented May 3, 2025

Uh oh!

oesteban commented May 3, 2025

Uh oh!

jhlegarreta commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhlegarreta commented May 31, 2025

Uh oh!

jhlegarreta Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhlegarreta Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

jhlegarreta Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

jhlegarreta commented Jun 14, 2025

Uh oh!

jhlegarreta Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oesteban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhlegarreta commented Jul 9, 2025

Uh oh!

Uh oh!

jhlegarreta commented May 3, 2025 •

edited

Loading

codecov bot commented May 3, 2025 •

edited

Loading

jhlegarreta commented May 8, 2025 •

edited

Loading

jhlegarreta Jun 14, 2025 •

edited

Loading

jhlegarreta Jun 14, 2025 •

edited

Loading