Skip to content

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Aug 13, 2025

Fix miscellaneous oversights in the base DWI model:

  • Provide the None default value for the n_jobs kwarg if not provided in the dictionary.

    Fixes:

            n_models = self._fit(
                index,
    >           n_jobs=kwargs.pop("n_jobs"),
                **kwargs,
            )
    E       KeyError: 'n_jobs'
    
  • Make the diffusion gradients have a unit norm in the gtab generation fixture.

    Fixes:

            bvecs = np.where(np.isnan(bvecs), 0, bvecs)
            bvecs_close_to_1 = abs(vector_norm(bvecs) - 1) <= atol
            if bvecs.shape[1] != 3:
                raise ValueError("bvecs should be (N, 3)")
            if not np.all(bvecs_close_to_1[dwi_mask]):
    >           raise ValueError(
                    "The vectors in bvecs should be unit (The tolerance "
                    "can be modified as an input parameter)"
                )
    E           ValueError: The vectors in bvecs should be unit (The tolerance can be modified as an input parameter)
    

Add a test to check that DIPY models (e.g. DTI) can be instantiated and that the returned prediction has the expected shape.

@jhlegarreta jhlegarreta requested a review from oesteban August 13, 2025 00:15
dtimodel = model.DTIModel(dataset)
predicted = dtimodel.fit_predict(4)

assert predicted.shape == dwi_dataobj.shape[:-1]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume we trust the prediction makes sense and that checking the shape here is enough.

@jhlegarreta jhlegarreta force-pushed the tst/test-dipy-models branch from 1f93599 to 69d8849 Compare August 13, 2025 00:18
Copy link

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.03%. Comparing base (bb3d129) to head (0a45a48).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   73.96%   76.03%   +2.06%     
==========================================
  Files          24       24              
  Lines        1402     1402              
  Branches      166      166              
==========================================
+ Hits         1037     1066      +29     
+ Misses        299      263      -36     
- Partials       66       73       +7     

☔ 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.

Fix miscellaneous oversights in the base DWI model:
- Provide the `None` default value for the `n_jobs` kwarg if not
  provided in the dictionary.

  Fixes:
  ```
          n_models = self._fit(
              index,
  >           n_jobs=kwargs.pop("n_jobs"),
              **kwargs,
          )
  E       KeyError: 'n_jobs'
  ```

- Make the diffusion gradients have a unit norm in the gtab generation
  fixture.

  Fixes:
  ```
          bvecs = np.where(np.isnan(bvecs), 0, bvecs)
          bvecs_close_to_1 = abs(vector_norm(bvecs) - 1) <= atol
          if bvecs.shape[1] != 3:
              raise ValueError("bvecs should be (N, 3)")
          if not np.all(bvecs_close_to_1[dwi_mask]):
  >           raise ValueError(
                  "The vectors in bvecs should be unit (The tolerance "
                  "can be modified as an input parameter)"
              )
  E           ValueError: The vectors in bvecs should be unit (The tolerance can be modified as an input parameter)
  ```

Add a test to check that DIPY models (e.g. DTI) can be instantiated and
that the returned prediction has the expected shape.
@jhlegarreta jhlegarreta force-pushed the tst/test-dipy-models branch from 69d8849 to 0a45a48 Compare August 13, 2025 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant