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

Suppress a warning in geometry. #1294

Merged
merged 5 commits into from Feb 9, 2018

Conversation

Projects
3 participants
@arokem
Member

arokem commented Jul 5, 2017

Another one of these annoying ones.

theta = np.arccos(z / r)
theta = np.where(r > 0, theta, 0.)

This comment has been minimized.

@skoudoro

skoudoro Jan 15, 2018

Member

Since Travis is not happy because of some round values, why not just put it all in one line?

theta = np.where(r > 0, np.arccos(z / r), 0.)

This comment has been minimized.

@skoudoro

skoudoro Jan 15, 2018

Member

or even better :

theta = np.arccos(z / r, where=r>0)
theta = np.where(r > 0, theta, 0.)

cool feature from Numpy! look here

@arokem arokem force-pushed the arokem:suppress_geometry_warning branch from fac7012 to f29c8c8 Jan 17, 2018

@skoudoro skoudoro added this to Needs a review in Dipy 0.14.0 Jan 17, 2018

# r is strictly positive by construction and will be 0 iff all three are
# zero, so it's safe to convert it to a 1 in this case, and prevents
# warnings from being raised in the following statement (z / r):
r = np.where(r > 0, r, 1)

This comment has been minimized.

@skoudoro

skoudoro Jan 17, 2018

Member

This part is not anymore necessary after the last changes :-)

This comment has been minimized.

@skoudoro

skoudoro Feb 8, 2018

Member

Any chance to see this PR in the future release? You just need to remove the lines from 129 to 132 and keep your change line 133.

This comment has been minimized.

@arokem

arokem Feb 8, 2018

Member

Oh - I didn't understand that was all that was left. Done and rebased!

@arokem arokem force-pushed the arokem:suppress_geometry_warning branch from 44f9576 to a527270 Feb 8, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Feb 8, 2018

Codecov Report

Merging #1294 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1294      +/-   ##
==========================================
- Coverage   86.82%   86.81%   -0.02%     
==========================================
  Files         243      243              
  Lines       30215    30215              
  Branches     3250     3250              
==========================================
- Hits        26235    26230       -5     
- Misses       3241     3244       +3     
- Partials      739      741       +2
Impacted Files Coverage Δ
dipy/core/geometry.py 90.29% <100%> (ø) ⬆️
dipy/reconst/forecast.py 90.15% <0%> (-2.08%) ⬇️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️

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 cb33390...384b010. Read the comment docs.

@arokem arokem referenced this pull request Feb 8, 2018

Merged

Suppress rcond warning #1419

@@ -126,7 +126,7 @@ def cart2sphere(x, y, z):
azimuth angle
'''
r = np.sqrt(x * x + y * y + z * z)
theta = np.arccos(z / r)
theta = np.arccos(z / r, where=r > 0)

This comment has been minimized.

@skoudoro

skoudoro Feb 9, 2018

Member

it seems we still have the warning. That's weird... So, I just tested another option:
np.arccos(np.divide(z, r, where=r > 0))
This solution seems to totally remove the warning.

This comment has been minimized.

@arokem

arokem Feb 9, 2018

Member

Let's see...

@arokem arokem force-pushed the arokem:suppress_geometry_warning branch from d0f7fc0 to 384b010 Feb 9, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 9, 2018

Yeah! this warning disappeared! merging!

Thank you @arokem !

@skoudoro skoudoro merged commit ec9b584 into nipy:master Feb 9, 2018

3 checks passed

codecov/patch 100% of diff hit (target 86.82%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +13.17% compared to cb33390
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Dipy 0.14.0 automation moved this from Needs a review to Done Feb 9, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment