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

MRG: Suppress a divide-by-zero warning. #1289

Merged
merged 4 commits into from Jul 10, 2017

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Jun 30, 2017

Suppress the warning described in #1181? Still needs testing.

@coveralls

This comment has been minimized.

coveralls commented Jun 30, 2017

Coverage Status

Coverage decreased (-0.002%) to 85.414% when pulling e42df69 on arokem:suppress_denom_warning into 0976d9e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 30, 2017

Coverage Status

Coverage decreased (-0.002%) to 85.414% when pulling e42df69 on arokem:suppress_denom_warning into 0976d9e on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 30, 2017

Codecov Report

Merging #1289 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
+ Coverage   87.11%   87.13%   +0.01%     
==========================================
  Files         228      228              
  Lines       28764    28800      +36     
  Branches     3090     3093       +3     
==========================================
+ Hits        25057    25094      +37     
  Misses       3003     3003              
+ Partials      704      703       -1
Impacted Files Coverage Δ
dipy/reconst/csdeconv.py 88.67% <ø> (+0.14%) ⬆️
dipy/direction/tests/test_peaks.py 99.44% <100%> (ø) ⬆️
dipy/reconst/tests/test_shore_odf.py 96.07% <100%> (+0.07%) ⬆️
dipy/reconst/tests/test_odf.py 92.98% <100%> (+1.67%) ⬆️
dipy/direction/peaks.py 79.32% <100%> (-0.49%) ⬇️
dipy/reconst/odf.py 93.33% <100%> (+0.74%) ⬆️
dipy/reconst/tests/test_mapmri.py 98.56% <100%> (-0.01%) ⬇️
dipy/reconst/tests/test_csdeconv.py 99.42% <0%> (+0.03%) ⬆️
dipy/core/gradients.py 97.36% <0%> (+0.04%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0976d9e...9cfa1df. Read the comment docs.

Import GFA from the ODF module, removing duplicate. Add testing.
Also, refactor, so that it returns a nan without warning for
voxels that have all-zero signal.

@arokem arokem changed the title from WIP: Suppress a divide-by-zero warning. to MRG: Suppress a divide-by-zero warning. Jul 5, 2017

@arokem

This comment has been minimized.

Member

arokem commented Jul 5, 2017

OK - this is ready for review.

@skoudoro

Some imports must be changed in :

  • test_shore_odf.py
  • test_mapmri.py

Otherwise, it looks good to me. Waiting for travis results

@@ -552,15 +551,6 @@ def peaks_from_model(model, data, sphere, relative_peak_threshold,
odf_array if return_odf else None)
def gfa(samples):
"""The general fractional anisotropy of a function evaluated

This comment has been minimized.

@skoudoro

skoudoro Jul 5, 2017

Member

it might be good to warn users / devs that the function moved in another module.

This comment has been minimized.

@arokem

arokem Jul 5, 2017

Member

This would not affect users (and I believe that it wouldn't even affect the aformentioned tests!), because we are now importing the name gfa in from reconst.odf into direction.peaks. So the API hasn't changed at all.

The only ones who might be surprised are developers who expect to find this function here. What do you propose to do to warn them about this?

This comment has been minimized.

@skoudoro

skoudoro Jul 5, 2017

Member

Oh ok, I see. Sorry and thanks ! No need to warn devs :-)

Even if it does not have any bad effect, I do not understand why you want to keep this "old" import from dipy.direction.peaks import gfa in the aformentioned test?

@arokem

This comment has been minimized.

Member

arokem commented Jul 5, 2017

OK - changed these imports

@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-3.8%) to 81.587% when pulling 7341ea4 on arokem:suppress_denom_warning into 0976d9e on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jul 5, 2017

Does anyone understand how coverage went down when I removed some code and added a test?! That doesn't make sense...

@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage increased (+0.02%) to 85.435% when pulling 9cfa1df on arokem:suppress_denom_warning into 0976d9e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage increased (+0.02%) to 85.435% when pulling 9cfa1df on arokem:suppress_denom_warning into 0976d9e on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 6, 2017

Coverage Status

Coverage increased (+0.02%) to 85.435% when pulling 9cfa1df on arokem:suppress_denom_warning into 0976d9e on nipy:master.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Jul 7, 2017

At the end, coverage went up ;-). It is more logic now. If there is no objection, I will merge it this monday (July 10th)

@skoudoro skoudoro merged commit 736cbb5 into nipy:master Jul 10, 2017

4 checks passed

codecov/patch 100% of diff hit (target 87.11%)
Details
codecov/project 87.13% (+0.01%) compared to 0976d9e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 85.435%
Details

arokem added a commit to arokem/dipy that referenced this pull request Jul 10, 2017

Revert addition of test here.
This was more comprehensively handled in nipy#1289

arokem added a commit to arokem/dipy that referenced this pull request Aug 9, 2017

Revert addition of test here.
This was more comprehensively handled in nipy#1289

arokem added a commit to arokem/dipy that referenced this pull request Oct 13, 2017

Revert addition of test here.
This was more comprehensively handled in nipy#1289

arokem added a commit to arokem/dipy that referenced this pull request Oct 13, 2017

Revert addition of test here.
This was more comprehensively handled in nipy#1289

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1289 from arokem/suppress_denom_warning
MRG: Suppress a divide-by-zero warning.

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Revert addition of test here.
This was more comprehensively handled in nipy#1289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment