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

Provide appropriate units for the return values of search_around_* #5528

Merged
merged 2 commits into from Dec 6, 2016

Conversation

@evertrol
Copy link
Contributor

evertrol commented Dec 1, 2016

The search_around_3d and search_around_sky functions and
methods previously returned an empty distance list with a unit of
dimensionless_unscaled when no match was found. This PR grabs
the distance unit from the first input argument and uses that instead.

@evertrol evertrol force-pushed the evertrol:units-returned-by-search_around_ branch from 50bba4d to 6ba5f3a Dec 1, 2016
@evertrol

This comment has been minimized.

Copy link
Contributor Author

evertrol commented Dec 1, 2016

PR to accompany the discussion in #5518 .

and ``idx2``; the unit is that of ``coords1``.
If either ``coords1`` or ``coords2`` don't have a distance,
this is the 3D distance on the unit sphere, rather than a
physical distance.

This comment has been minimized.

Copy link
@mhvk

mhvk Dec 1, 2016

Contributor

True nitpicking, but what if coords1 has a distance and coords2 has not, and the result is empty? In that case, should the unit be dimensionless?

This comment has been minimized.

Copy link
@evertrol

evertrol Dec 1, 2016

Author Contributor

Certainly good to ask, and the reason why I put in a PR, because there will be a bunch of corner cases like this that may warrant some discussion.
If we'd like to stick to the convention stated in the docstring here, there should indeed be a check for the distance of coords2 when either the input as in line 331 or the result as in line 373 is empty.

evertrol added 2 commits Dec 1, 2016
The ``search_around_3d`` and ``search_around_sky`` functions and
methods previously returned an empty distance list with a unit of
``dimensionless_unscaled`` when no match was found. This commit grabs
the distance unit from the first input argument and uses that instead.
@evertrol evertrol force-pushed the evertrol:units-returned-by-search_around_ branch from 6ba5f3a to ab3619e Dec 5, 2016
@eteq
eteq approved these changes Dec 6, 2016
Copy link
Member

eteq left a comment

Looks great! Will merge this to make sure it's in for 1.3

@eteq eteq merged commit a6dcf8b into astropy:master Dec 6, 2016
3 checks passed
3 checks passed
ci/circleci 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
@evertrol evertrol deleted the evertrol:units-returned-by-search_around_ branch Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.