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

MNT: Remove deprecated dms_to_degrees and hms_to_hours #15205

Merged
merged 2 commits into from Aug 25, 2023
Merged

MNT: Remove deprecated dms_to_degrees and hms_to_hours #15205

merged 2 commits into from Aug 25, 2023

Conversation

pllim
Copy link
Member

@pllim pllim commented Aug 19, 2023 •
edited

Description

This pull request is to fix #13179

p.s. Local tests failed miserably. Maybe a coordinates maintainer should take over. It does not appear to be trivial to get rid of these.

  • 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.

@pllim pllim added this to the v6.0 milestone Aug 19, 2023
@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.

@github-actions
Copy link

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@pllim pllim requested review from eteq and mhvk August 19, 2023 01:58
@pllim pllim mentioned this pull request Aug 19, 2023
Closed
@pllim pllim force-pushed the deprecate-coordinates-stuff branch 2 times, most recently from 488e7e2 to d996820 Compare August 23, 2023 18:04
@pllim pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Aug 23, 2023
@pllim pllim marked this pull request as ready for review August 23, 2023 18:27
@pllim
Copy link
Member Author

pllim commented Aug 23, 2023

The diff is a bit more drastic than I thought from the original issue description, but I think the changes make sense. CI is green now, please review. Should get this in sooner than later so downstream has time to adapt. Thanks!

eerovaher
eerovaher previously requested changes Aug 23, 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.

This looks like a good cleanup, but the pull request contains unrelated changes (even if I agree that those changes are for the better) and instead of NotImplementedError we should be using TypeError.

astropy/coordinates/angles.py Outdated Show resolved Hide resolved
astropy/coordinates/tests/test_angles.py
Comment on lines -103 to +97
a = Angle("12 32 99", unit=u.degree)
Angle("12 32 99", unit=u.degree)
Copy link
Member

Choose a reason for hiding this comment

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

Not assigning values to variables that never get used is a good change, but it shouldn't be included in the commit that removes deprecated code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a refactoring that will never get backported, so I don't see a big deal. I can undo these changes but I probably won't come back to do just this clean up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy for you to leave them in, since you've done them. A new PR is enough of a pain that sometimes one just fixes up little things in unrelated ones... (well, I certainly do, even though I sometimes complain about it too!!)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I am keeping these then. Sorry for the eye sore. I will try to control myself better next time.

Copy link
Member

Choose a reason for hiding this comment

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

Opening a separate PR for this should not be necessary, but I'd rather move these to a separate commit myself than keep everything in a single commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather move these to a separate commit myself than keep everything in a single commit.

That is even more work for me than just removing them. It is Friday and I want to move this off my todo. Should I just remove the changes you don't like even though you agree they are good changes?

Copy link
Member

Choose a reason for hiding this comment

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

For many of these tests the best thing to do (but clearly beyond the scope of this pull request) would be to rewrite them as parametrized tests. For doing that the changes in question will not be important and applying them would create code churn. So if you want to undo these changes then that's fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eerovaher - I think you maybe take @pllim too literally: we all agree these changes are good, but differ on whether it is worth the work and CI time to split things up in separate PRs or commits just so the commit history is a bit cleaner. @pllim wants to be done (and I'd rather she have as much time as possible to continue to keep things afloat!), and feels this is not worth more work, while you seem to feel differently. While I like how you have made astropy better in having single, well-contained commits and PRs, here to me the effort-to-value ratio (in both work and CI time and thus CO2) really seems rather low. So, I suggest we just merge this. Or maybe you split how you like it and force-push? I'm sure @pllim would be OK with that (we've done that mutually often enough, just trying to optimize time spent).

astropy/coordinates/tests/test_arrays.py Outdated Show resolved Hide resolved
astropy/coordinates/tests/test_angles.py Outdated Show resolved Hide resolved
docs/changes/coordinates/15205.api.rst Outdated Show resolved Hide resolved
mhvk
mhvk reviewed Aug 24, 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.

Thanks, @pllim! Annoying that this become more of a drag... Overall, looks good. My main comment is about the error message, plus a different suggestion for cleaning up the tests (though feel free to pick either @eerovaher's or mine).

astropy/coordinates/angles.py Outdated Show resolved Hide resolved
astropy/coordinates/tests/test_angles.py Outdated Show resolved Hide resolved
astropy/coordinates/tests/test_angles.py
Comment on lines -103 to +97
a = Angle("12 32 99", unit=u.degree)
Angle("12 32 99", unit=u.degree)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy for you to leave them in, since you've done them. A new PR is enough of a pain that sometimes one just fixes up little things in unrelated ones... (well, I certainly do, even though I sometimes complain about it too!!)

astropy/coordinates/tests/test_angles.py Outdated Show resolved Hide resolved
astropy/coordinates/tests/test_angles.py Outdated Show resolved Hide resolved
@pllim
Copy link
Member Author

pllim commented Aug 24, 2023

I think I have addressed all the comments. Please re-review. Thanks!

@pllim

This comment was marked as resolved.

Sign in to view
mhvk
mhvk approved these changes Aug 24, 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.

Looks good to me!

@pllim
Copy link
Member Author

pllim commented Aug 24, 2023

@eerovaher , are you okay to approve now or is there something else?

@eerovaher
Copy link
Member

I followed the suggestion of @mhvk and split this pull request into sensible commits myself. As I've explained earlier 58d287b is optional and it's fine to merge this pull request with or without it.

@eerovaher eerovaher dismissed their stale review August 25, 2023 18:34

I applied the requested changes myself.

@mhvk mhvk added 💤 merge-when-ci-passes Do not use: We have auto-merge option now. and removed 💤 merge-when-ci-passes Do not use: We have auto-merge option now. labels Aug 25, 2023
@mhvk mhvk enabled auto-merge August 25, 2023 18:47
@mhvk
Copy link
Contributor

mhvk commented Aug 25, 2023

Great! Deprecated stuff removed and tests at least somewhat improved. Enabling auto-merge.

@mhvk mhvk merged commit 1d11e15 into astropy:main Aug 25, 2023
29 of 30 checks passed
@pllim pllim deleted the deprecate-coordinates-stuff branch August 25, 2023 19:19
@pllim
Copy link
Member Author

pllim commented Aug 25, 2023

Aww, thank you so much for cleaning up after me. It is nice to see this merge after coming back from school event. Thanks again, you two!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk approved these changes

@eerovaher eerovaher eerovaher left review comments

@eteq eteq Awaiting requested review from eteq

Assignees
No one assigned
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period coordinates Refactoring
Projects
None yet
Milestone
v6.0.0
Development

Successfully merging this pull request may close these issues.

MNT: Remove deprecated dms_to_degrees and hms_to_hours
3 participants
@pllim @eerovaher @mhvk