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 how coordinates handles negative or nan parallaxes/distances #7988

Merged
merged 12 commits into from Oct 28, 2018

Conversation

Projects
None yet
4 participants
@adrn
Member

adrn commented Oct 26, 2018

This addresses a few issues related to how negative parallaxes / distances are handled in coordinates.

with master:

image

with this PR:

image

Fixes #7408

@adrn adrn added this to the v3.1 milestone Oct 26, 2018

@adrn adrn requested review from eteq and mhvk Oct 26, 2018

@astropy-bot

This comment has been minimized.

astropy-bot bot commented Oct 26, 2018

Hi there @adrn 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@adrn adrn force-pushed the adrn:coordinates/negative-parallax branch from 548864a to 9429a34 Oct 26, 2018

@adrn adrn changed the title from Coordinates/negative parallax to Fix how coordinates handles negative or nan parallaxes/distances Oct 26, 2018

@adrn

This comment has been minimized.

Member

adrn commented Oct 26, 2018

Failures are real! Apparently, in master, you can do:

>>> rep = coord.SphericalRepresentation(160*u.deg, 10*u.deg, 10*u.pc)
>>> -rep
<SphericalRepresentation (lon, lat, distance) in (deg, deg, pc)
    ( 160.,  10., -10.)>

Because of this change, that no longer works:

>>> rep = coord.SphericalRepresentation(160*u.deg, 10*u.deg, 10*u.pc)
>>> -rep
NotImplemented

I actually feel like the behavior in this PR is more "correct", in the sense that a negative distance doesn't mean anything, so we might want to either not allow this operation (i.e. raise a NotImplementedError error?), or special case it and do from_cartesian(-rep.to_cartesian()). Thoughts?

@eteq

This comment has been minimized.

Member

eteq commented Oct 26, 2018

I think I prefer the "special-casing" option, @adrn. That's actually more consistent with what representations do anyway, right?

@adrn

This comment has been minimized.

Member

adrn commented Oct 26, 2018

Well, kind of. For any Representation object, it uses _scale_operation(), which says "Scale all non-angular components, leaving angular ones unchanged." But, doing from_cartesian(-rep.to_cartesian()) will actually change the angular components.

@mhvk

This comment has been minimized.

Contributor

mhvk commented Oct 26, 2018

I think we need to keep -rep working - didn't I let angles change in UnitSpherical as well? Maybe just reproduce that.

@eteq

eteq approved these changes Oct 28, 2018

Just a few minor suggestions. They're straightforward enough I'll just implement myself and then merge

"distances even when `allow_negative=True`, "
"because negative parallaxes cannot be transformed "
"into distances. See discussion in this paper: "
"https://arxiv.org/abs/1507.02105", AstropyWarning)

This comment has been minimized.

@eteq

eteq Oct 28, 2018

Member

Nice! An actual reference!

"https://arxiv.org/abs/1507.02105", AstropyWarning)
else:
raise ValueError("Negative parallaxes are not "
"interpretable as distances. If you "

This comment has been minimized.

@eteq

eteq Oct 28, 2018

Member

Add the citation to the paper here too, perhaps?

self._distance = self._distance.view(Distance)
try:
self._distance = Distance(self._distance, copy=False)
except ValueError:

This comment has been minimized.

@eteq

eteq Oct 28, 2018

Member

There may be other sources of ValueError here. Easiest thing to do is just check that the message mentions the distance being >=0...?

@eteq eteq merged commit ed28de6 into astropy:master Oct 28, 2018

12 checks passed

astropy-bot All checks passed
Details
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl202 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl222 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl300 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpldev Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment