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

fix RuntimeWarning for nan values #367

Merged
merged 1 commit into from
Aug 28, 2021

Conversation

cshanahan1
Copy link
Contributor

This addresses #366

Copy link
Contributor

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

Instead of changing the data, which could have adverse side-effects, I think it might be better to allow the data to pass through as nans and then capture the warning, especially if it is expected as in this case. So something like:

import warnings

    with warnings.catch_warnings():
        warnings.filterwarnings(action="ignore",
                                message="invalid value encountered in remainder",
                                category=RuntimeWarning)
        lon = np.mod(lon, 360.0 * u.deg if nquant else 360.0)

Which uses a context manager to apply a very specific warnings ignore filter just within the with block.

@jdavies-st jdavies-st added this to the 0.17 milestone Jun 7, 2021
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #367 (cb7932e) into master (8b8a2bd) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head cb7932e differs from pull request most recent head 7991e3b. Consider uploading reports for the commit 7991e3b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   86.34%   86.35%   +0.01%     
==========================================
  Files          21       21              
  Lines        3624     3627       +3     
==========================================
+ Hits         3129     3132       +3     
  Misses        495      495              
Impacted Files Coverage Δ
gwcs/geometry.py 98.01% <100.00%> (+0.06%) ⬆️

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 8b8a2bd...7991e3b. Read the comment docs.

Copy link
Contributor

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

Looks good. Just needs a change log entry.

Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

Needs a change log

gwcs/geometry.py Outdated
@@ -200,7 +201,13 @@ def evaluate(self, x, y, z):
lon[h == 0] *= 0

if self._wrap_lon_at != 180:
lon = np.mod(lon, 360.0 * u.deg if nquant else 360.0)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is more readable if instead of catching the warning the call is modified as in

lon = np.mod(lon, 360.0 * u.deg if nquant else 360.0, where=np.isfinite(lon), out=lon)

@nden nden closed this Aug 28, 2021
@nden nden reopened this Aug 28, 2021
@nden nden merged commit 0f3d750 into spacetelescope:master Aug 28, 2021
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.

3 participants