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

Rename Fits class in units.format to FITS #16455

Merged
merged 1 commit into from
May 14, 2024

Conversation

eerovaher
Copy link
Member

Description

Many unit formatter classes are named exactly (i.e. including capitalization) like the standard that they implement (e.g. OGIP or VOUnit), but the class implementing FITS unit formatting is called Fits instead of FITS. This is proving to be very inconvenient in #16445, which is reducing code duplication in the units.format.Generic subclasses by replacing some of their individual methods with common implementations in Generic. In particular, if the _validate_unit() class method raises an error or a warning then the message should refer to the relevant unit standard, but using cls.__name__ is not suitable because Fits has the wrong capitalization and cls.__name__.upper() would ruin the capitalization of VOUnit. This pull request will therefore simplify #16445, but it is better to have separate pull requests for API changes and user-invisible refactoring.

The renaming should have a very low impact for users because the unit formatter classes are not meant to be imported or used directly anyways.

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

@eerovaher eerovaher added units API change PRs and issues that change an existing API, possibly requiring a deprecation period labels May 14, 2024
@eerovaher eerovaher added this to the v7.0.0 milestone May 14, 2024
@eerovaher eerovaher requested a review from mhvk May 14, 2024 15:23
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?
  • 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.

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, this is great! I only have a suggestion for the changelog message - I think we should stress this was a private class. Indeed, arguably no changelog entry is needed, but we might as well be nice and have one.

@@ -0,0 +1,2 @@
The ``format.Fits`` formatter class has been renamed to ``format.FITS``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest:

The private ``astropy.units.format.Fits`` formatter class has been renamed to ``format.FITS``.
The old name is deprecated. Note that this does not affect regular user visible behaviour:
the formatters are selected indirectly via their lower-case name (in this case ``format="fits"``).
This change should have no effect except for those who may have subclassed the formatter.

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 expanded the change log entry, but not quite how you suggested. Mentioning format="fits" seems to assume the use of the to_string() method, but the formats can also be used in an f-string (e.g. f"{q:fits}"), in which case there is no explicit format argument.

Making the class name uppercase ensures it consistent with the CDS and
OGIP unit formatter classes. The renaming should not impact users
because the formatter classes are not meant to be imported directly.
@eerovaher eerovaher force-pushed the mv-units-formatter-Fits-FITS branch from f1737f4 to 086ccac Compare May 14, 2024 17:13
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.

I may have preferred for the changelog to mention explicitly that this class was a private one - it is not exposed under astropy.units, but don't want to let the perfect be the enemy of good enough - so let's get this in! Thanks!

@mhvk mhvk merged commit 6e34d6d into astropy:main May 14, 2024
27 checks passed
@eerovaher eerovaher deleted the mv-units-formatter-Fits-FITS branch May 14, 2024 21:43
@eerovaher
Copy link
Member Author

The unit formatter classes are a bit strange because they are not available from the astropy.units namespace (as if they are private), but they are included in the units API documentation (as if they are public).

@mhvk
Copy link
Contributor

mhvk commented May 14, 2024

Yes, that's a good point; definitely good then to have the changelog entry! (and your careful deprecation message)

@pllim
Copy link
Member

pllim commented May 22, 2024

Are you sure we should do this? It broke specutils via gwcs, see https://github.com/astropy/specutils/actions/runs/9183362928/job/25253934562

gwcs/wcs.py:2035: in _separable_groups
    cunit = frame.unit[fidx].get_format_name(u.format.Fits).upper()

cc @rosteen @nden

@neutrinoceros
Copy link
Contributor

Looks simple enough to address downstream. I'll open a PR later today unless anyone beats me to it :)

@pllim
Copy link
Member

pllim commented May 22, 2024

OK thanks. I opened spacetelescope/gwcs#499

@eerovaher
Copy link
Member Author

The gwcs code mentioned here is buggy. I will explain it in the issue that has been opened there because the bug doesn't really have anything to do with this pull request.

@eerovaher
Copy link
Member Author

I will just mention here that NamedUnit.get_format_name() makes no attempts to check if its input is a valid format or not. Maybe it could perform some validation, but if we decide that it should then that's a completely separate issue from renaming the FITS formatter class and it should not be discussed here any further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period units
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants