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.

Sign up for GitHub

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

Jump to bottom

Allow initializing Distance from angle-like parallax #15712

Merged
merged 2 commits into from Dec 12, 2023
Merged

Allow initializing Distance from angle-like parallax #15712

merged 2 commits into from Dec 12, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Dec 11, 2023 •
edited by eerovaher

Description

This pull request is to allow initializing Distance from an angle-like parallax which is not a Quantity. In particular any numeric object with an angular unit will work.

Fixes #15693

- [ ] By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@taldcroft taldcroft added this to the v6.0.1 milestone Dec 11, 2023
@github-actions GitHub Actions
Copy link

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@eerovaher eerovaher added Bug backport-v6.0.x on-merge: backport to v6.0.x labels Dec 11, 2023
eerovaher
eerovaher previously requested changes Dec 11, 2023
View reviewed changes
astropy/coordinates/distances.py Show resolved Hide resolved
astropy/coordinates/distances.py Outdated Show resolved Hide resolved
astropy/coordinates/tests/test_distance.py Outdated Show resolved Hide resolved
astropy/coordinates/tests/test_distance.py Outdated Show resolved Hide resolved
docs/changes/coordinates/15712.bugfix.rst Outdated Show resolved Hide resolved
@taldcroft taldcroft force-pushed the coords-allow-angle-like-in-distance branch from 03d46b7 to 288d07e Compare December 11, 2023 22:09
@taldcroft
Copy link
Member Author

@eerovaher - thanks for the great suggestions, I applied them all and tests are running now. I rebased on main hoping that docs will build OK.

eerovaher
eerovaher reviewed Dec 12, 2023
View reviewed changes
Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

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

The updates to the code look good, but I would like bugfix pull requests to have two commits. The first would add the regression test, the second the fix itself and the change log entry. Checking out the first commit would then allow to very easily verify both that the bug exists and that the new regression test catches it. I am willing to rebase the pull request myself to get the commit structure I'd like.

@taldcroft
Copy link
Member Author

@eerovaher - go ahead on the rebase if you like. Personally I think that is not the most productive use of your valuable time since we have the original bug report which I have independently confirmed. As a general policy I don't think that splitting tests and code updates is going to be workable.

@taldcroft @eerovaher
Add a regression test for non-Quantity parallax
a65a0e6 
The new test reveals that passing in an angle-like object as parallax to
`coordinates.Distance` does not always work. Most notable example is
`table.Column`.
@eerovaher eerovaher force-pushed the coords-allow-angle-like-in-distance branch from 288d07e to c16a408 Compare December 12, 2023 16:01
@taldcroft @eerovaher
Allow using any angle-like parallax with Distance
38f10a3 
The `parallax` argument to `Distance` can now be any object that can be
converted to an angular `Quantity`, most notably `table.Column`.
@eerovaher eerovaher force-pushed the coords-allow-angle-like-in-distance branch from c16a408 to 38f10a3 Compare December 12, 2023 16:03
@eerovaher eerovaher dismissed their stale review December 12, 2023 16:06

Suggestions have been implemented

@eerovaher eerovaher requested a review from mhvk December 12, 2023 16:06
mhvk
mhvk approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@mhvk mhvk merged commit 35c1cc2 into astropy:main Dec 12, 2023
24 of 26 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Dec 12, 2023
@mhvk @meeseeksmachine
Backport PR astropy#15712: Allow initializing Distance from angle-lik…
ce732b8 
…e parallax
@meeseeksmachine meeseeksmachine mentioned this pull request Dec 12, 2023
Merged
pllim added a commit that referenced this pull request Dec 12, 2023
@pllim
Merge pull request #15724 from meeseeksmachine/auto-backport-of-pr-15…
d87b1b9 
…712-on-v6.0.x

Backport PR #15712 on branch v6.0.x (Allow initializing Distance from angle-like parallax)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerovaher eerovaher eerovaher left review comments

@mhvk mhvk mhvk approved these changes

Assignees
No one assigned
Labels
backport-v6.0.x on-merge: backport to v6.0.x Bug coordinates
Projects
None yet
Milestone
v6.0.1
Development

Successfully merging this pull request may close these issues.

astropy.coordinates.Distance does not work with parallax values in a table Column with units
3 participants
@taldcroft @mhvk @eerovaher