Skip to content

Ensure angle wrapping of NaN does not raise a warning. - #11568

Merged
mhvk merged 2 commits into
astropy:mainfrom
mhvk:coordinates-angles-wrapping-with-nan
Apr 19, 2021
Merged

Ensure angle wrapping of NaN does not raise a warning.#11568
mhvk merged 2 commits into
astropy:mainfrom
mhvk:coordinates-angles-wrapping-with-nan

Conversation

@mhvk

@mhvk mhvk commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

Also fix some small rounding problems. But consolidates routines between Angle and Longitude.

fixes #11558

@mhvk mhvk added this to the v4.3 milestone Apr 14, 2021
mhvk
mhvk commented Apr 14, 2021
View reviewed changes
Comment thread astropy/coordinates/angles.py Outdated
@mhvk mhvk mentioned this pull request Apr 14, 2021
Merged
nstarman
nstarman reviewed Apr 14, 2021
View reviewed changes
Comment thread astropy/coordinates/angles.py
@mhvk
mhvk force-pushed the coordinates-angles-wrapping-with-nan branch from a897d07 to 72ce4e3 Compare April 15, 2021 00:21
nstarman
nstarman reviewed Apr 15, 2021
View reviewed changes
Comment thread astropy/coordinates/angles.py Outdated
Comment thread astropy/coordinates/angles.py Outdated
Comment thread astropy/coordinates/angles.py
@mhvk
mhvk force-pushed the coordinates-angles-wrapping-with-nan branch from 72ce4e3 to 4c9a270 Compare April 15, 2021 17:06
@mhvk

mhvk commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

Thanks! I now also added an actual regression test for #11558

@mhvk
mhvk force-pushed the coordinates-angles-wrapping-with-nan branch 2 times, most recently from ca285e6 to d3d108a Compare April 15, 2021 17:31
@mhvk

mhvk commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

Oops still had a problem with nan comparisons. Now hopefully fixed. Also rebased so that hopefully all tests will be green.

@nstarman

nstarman commented Apr 15, 2021

Copy link
Copy Markdown
Member

It still has the problem identified in #11558, but this resolves the wrap_angle NaN problems.

@mhvk

mhvk commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

Oops, you are right, on older numpy the comparisons also are a problem and thus raise a warning. That means this doesn't quite fix #11558 (as you note) - my note saying otherwise was just because I tested your example on a more recent numpy...

Let me add that fix too....

@mhvk
mhvk force-pushed the coordinates-angles-wrapping-with-nan branch from d3d108a to a1a598f Compare April 15, 2021 19:26
@mhvk mhvk mentioned this pull request Apr 15, 2021
Closed
@mhvk

mhvk commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

@adrn - @nstarman already reviewed this, but it also needs a maintainer. Relatively straightforward and will help get #11444 in... Thanks!

adrn
adrn approved these changes Apr 19, 2021
View reviewed changes

@adrn adrn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR looks good to me! I have one comment about overzealous wrapping, but may have just forgotten the reasoning: if so, feel free to ignore, otherwise perhaps we should discuss next Monday at the coordinates call or make an issue to discuss?

Comment thread astropy/coordinates/representation.py
Comment on lines +1975 to +1976
# Note that _phi already holds our own copy if copy=True.
self._phi.wrap_at(360 * u.deg, inplace=True)

@adrn adrn Apr 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is out of scope for this PR, but wondering now why we have to do this? Even in SphericalRepresentation, the longitude value gets wrapped by default because it gets passed into Longitude, which has no way to disable wrapping. But related to our discussions about internal representation of coordinate data and how we display it in frames, I wonder if we really need to wrap the data on input?

@mhvk mhvk Apr 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, a good point, and it is a bit strange that we enforce the wrap angle on the user. On the other hand, for the latitude we do decide that it is between 0 and 180 (here) or 90 and -90 (for spherical). And if one converts from Cartesian one does get these specific ranges too. Anyway, my sense is to let this be for now (and certainly for this PR!).

@mhvk

mhvk commented Apr 19, 2021

Copy link
Copy Markdown
Contributor Author

OK, thanks for the review, @adrn. I'll merge so @nstarman can wrap up the .transform() bit.

@mhvk
mhvk merged commit 3fa911b into astropy:main Apr 19, 2021
@mhvk
mhvk deleted the coordinates-angles-wrapping-with-nan branch April 19, 2021 21:54
@ayshih ayshih mentioned this pull request Oct 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adrn adrn adrn approved these changes
@nstarman nstarman nstarman approved these changes

Assignees

No one assigned

Labels

Bug coordinates

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

Creating a PhysicsSphericalRepresentation with NaNs raises RuntimeWarning

3 participants

@mhvk @nstarman @adrn