Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Deal appropriately with user warnings #2478

Merged
merged 3 commits into from Nov 6, 2021

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Nov 4, 2021

  • ENH: Appropriately deal with user warnings in tests
  • STYLE: Improve the warning message style
  • DOC: Fix method name typo in deprecation warning

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Nov 4, 2021

@codecov
Copy link

codecov bot commented Nov 5, 2021

Codecov Report

Merging #2478 (db632c6) into master (c22e18d) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2478      +/-   ##
==========================================
- Coverage   81.96%   81.95%   -0.01%     
==========================================
  Files         125      125              
  Lines       16637    16637              
  Branches     2703     2703              
==========================================
- Hits        13636    13635       -1     
  Misses       2373     2373              
- Partials      628      629       +1     
Impacted Files Coverage Δ
dipy/core/gradients.py 91.03% <100.00%> (ø)
dipy/reconst/shm.py 93.06% <100.00%> (ø)
dipy/reconst/forecast.py 89.84% <0.00%> (-0.51%) ⬇️

@arokem
Copy link
Contributor

arokem commented Nov 5, 2021

LGTM. The CI failures are unrelated, right?

@jhlegarreta
Copy link
Contributor Author

LGTM. The CI failures are unrelated, right?

Updated my previous comment with the additional two builds that failed. CI failures seem unrelated to me.

@jhlegarreta
Copy link
Contributor Author

Had left one warning behind. Push-forced.

@jhlegarreta jhlegarreta force-pushed the DealAppropriatelyWithUserWarnings branch from 254c09e to 4088f4a Compare November 5, 2021 14:30
@jhlegarreta jhlegarreta force-pushed the DealAppropriatelyWithUserWarnings branch 2 times, most recently from 6ee3500 to 36e0b9d Compare November 5, 2021 17:48
@jhlegarreta
Copy link
Contributor Author

Rebased on master to fix merge conflicts.

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Thanks @jhlegarreta for this. I have just a small comment below

dipy/reconst/tests/test_ivim.py Outdated Show resolved Hide resolved
Appropriately deal with user warnings in tests:
- Avoid unnecessarily raising user warnings.
- Catch user warnings where required.

Avoids unnecessarily displaying warnings:
```
reconst/tests/test_ivim.py::test_single_voxel_fit
  /home/vsts/work/1/s/venv/lib/python3.9/site-packages/dipy/core/gradients.py:300:
UserWarning:
b0_threshold (value: 0) is too low, increase your              b0_threshold. It should be higher than the lowest b0 value              (5.0).
    warn("b0_threshold (value: {0}) is too low, increase your \
```

and
```
reconst/tests/test_mcsd.py::test_mask_for_response_msmt_nvoxels
  /home/vsts/work/1/s/venv/lib/python3.9/site-packages/dipy/reconst/mcsd.py:590: UserWarning:
Some b-values are higher than 1200.
          The DTI fit might be affected.
    warnings.warn(msg_bvals, UserWarning)
```

and
```
reconst/tests/test_shm.py::test_real_sym_sh_mrtrix
  /home/vsts/work/1/s/venv/lib/python3.9/site-packages/dipy/reconst/tests/test_shm.py:120:
DeprecationWarning: dipy.reconst.shm.real_sym_sh_mrtix is deprecated, Please use dipy.reconst.shm.real_sh_tournier instead
```

Visible, for example, in:
https://dev.azure.com/dipy/dipy/_build/results?buildId=1340&view=logs&j=b82538f6-45b4-59fa-d676-9f3844eed7f4&t=6b675e20-6c23-5053-d81a-1533e41542cf&l=5362
and
https://dev.azure.com/dipy/dipy/_build/results?buildId=1340&view=logs&j=b82538f6-45b4-59fa-d676-9f3844eed7f4&t=6b675e20-6c23-5053-d81a-1533e41542cf&l=5372
and
https://dev.azure.com/dipy/dipy/_build/results?buildId=1340&view=logs&j=b82538f6-45b4-59fa-d676-9f3844eed7f4&t=6b675e20-6c23-5053-d81a-1533e41542cf&l=5386
Improve the warning message style: avoid whitespaces due to string
literal line breaks in code.

Allows to show:
```
UserWarning: b0_threshold (value: 0) is too low, increase your b0_threshold. It should be higher than the lowest b0 value (5.0).
  warn("b0_threshold (value: {0}) is too low, increase your "
```

instead of:
```
UserWarning: b0_threshold (value: 0) is too low, increase your              b0_threshold. It should be higher than the lowest b0 value              (5.0).
    warn("b0_threshold (value: {0}) is too low, increase your \
```
Fix method name typo in deprecation warning.
@jhlegarreta jhlegarreta force-pushed the DealAppropriatelyWithUserWarnings branch from 36e0b9d to db632c6 Compare November 5, 2021 18:15
@skoudoro skoudoro merged commit 7ca2d6c into dipy:master Nov 6, 2021
@skoudoro
Copy link
Member

skoudoro commented Nov 6, 2021

thank you @jhlegarreta!

@jhlegarreta jhlegarreta deleted the DealAppropriatelyWithUserWarnings branch November 6, 2021 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants