Allow controlling warnings when converting deprecated units to strings by eerovaher · Pull Request #18586 · astropy/astropy

eerovaher · 2025-09-02T21:58:47Z

Description

The "vounit" and "ogip" unit formats have deprecated units and converting them to strings raises warnings that currently cannot be avoided. The first commit introduces a new deprecations parameter to the UnitBase and unit formatter to_string() methods which allows silencing those warnings or replacing them with errors. This is analogous to the parse_strict parameter the unit parsing methods have. The default is to warn, which is how astropy behaves right now, but it is also sensible default behavior.

The second commit introduces a fourth "convert" option, which tells astropy to automatically replace deprecated units. If that is not possible then the deprecated unit is written out and a warning is emitted. In practice, with "vounit" automatic replacement is always possible and with "ogip" it is never possible.

The four options are implemented as members of a StrEnum, but the enum is not exposed to the users yet, nor is it explicitly used in the tests. This implementation demonstrates that we can start using enums internally, so that when we do make them public the amount of code changes will be smaller. Furthermore, by using StrEnum we can update the public API without breaking backwards compatibility in practice. The enum is implemented in a separate module so that it (and, in the future, other enums) could be imported without having to worry about import loops.

I briefly considered adding a new method to UnitBase instead of adding a fourth option for handling deprecated units, but using that design would look something like

u.erg.replace_deprecated("vounit").to_string(format="vounit")

i.e. the user would have to specify the unit format twice. Such repetition should be avoided, so the deprecations="convert" option is better.

Closes #18304

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.

github-actions · 2025-09-02T21:58:59Z

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.

github-actions · 2025-09-02T21:59:00Z

👋 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?

eerovaher · 2025-09-11T15:16:15Z

+If automatic replacement is not possible then the deprecated unit is used for
+constructing the string and a warning is emitted.


It would be good to add a code example, but our public API does not directly expose any units suitable for this purpose and parsing a string would emit a different warning. In the unit tests I avoided that warning by going through private API, but in user documentation that is unacceptable.

Not the prettiest, but in principle one could do,

from astropy.units import cds cds.Crab.to_string(format="ogip")

p.s. It is indeed a bit annoying that u.Unit("Crab", format="ogip", parse_strict="silent") actually is not silent...

Using cds here would make the example too confusing.

nstarman

A nice cleanup.

nstarman · 2025-09-11T15:35:12Z

        ----------
        unit : |Unit|
            The unit to convert.
+        deprecations : {"warn", "silent", "raise", "convert"}, optional, keyword-only


Are strings permitted?

It is best if these options do not differ needlessly from the parse_strict options the unit parsing methods have, and those are all strings.

Then are the type annotations correct?

The tests and documentation only use strings.

nstarman · 2025-09-11T15:48:13Z

+        if s in cls._deprecated_units and deprecations != DeprecatedUnitAction.SILENT:
            alternative = (
                unit.represents if isinstance(unit := cls._units[s], Unit) else None
            )
            msg = f"The unit {s!r} has been deprecated in the {cls.__name__} standard."
            if alternative:
+                if deprecations == DeprecatedUnitAction.CONVERT:
+                    return alternative
                msg += f" Suggested: {cls.to_string(alternative)}."
-            warnings.warn(msg, UnitsWarning)
+
+            if deprecations == DeprecatedUnitAction.RAISE:
+                raise UnitsError(msg)
+            if deprecations == DeprecatedUnitAction.WARN:
+                warnings.warn(msg, UnitsWarning)
+            elif deprecations == DeprecatedUnitAction.CONVERT:
+                warnings.warn(
+                    msg + " It cannot be automatically converted.", UnitsWarning
+                )
+            else:
+                valid_options = ", ".join(map(repr, map(str, DeprecatedUnitAction)))
+                raise ValueError(
+                    f"invalid deprecation handling option: {deprecations!r}. Valid "
+                    f"options are {valid_options}."
+                )


Suggested change

if s in cls._deprecated_units and deprecations != DeprecatedUnitAction.SILENT:

alternative = (

unit.represents if isinstance(unit := cls._units[s], Unit) else None

)

msg = f"The unit {s!r} has been deprecated in the {cls.__name__} standard."

if alternative:

if deprecations == DeprecatedUnitAction.CONVERT:

return alternative

msg += f" Suggested: {cls.to_string(alternative)}."

warnings.warn(msg, UnitsWarning)

if deprecations == DeprecatedUnitAction.RAISE:

raise UnitsError(msg)

if deprecations == DeprecatedUnitAction.WARN:

warnings.warn(msg, UnitsWarning)

elif deprecations == DeprecatedUnitAction.CONVERT:

warnings.warn(

msg + " It cannot be automatically converted.", UnitsWarning

)

else:

valid_options = ", ".join(map(repr, map(str, DeprecatedUnitAction)))

raise ValueError(

f"invalid deprecation handling option: {deprecations!r}. Valid "

f"options are {valid_options}."

)

if s in cls._deprecated_units and deprecations != DeprecatedUnitAction.SILENT:

if isinstance(unit := cls._units[s], Unit):

if deprecations == DeprecatedUnitAction.CONVERT:

return unit.represents

msg = (

DEPRECATED_UNIT_MSG.format(s, cls.__name__)

+ f" Suggested: {cls.to_string(unit.represents)}."

)

else:

msg = DEPRECATED_UNIT_MSG.format(s, cls.__name__)

match DeprecatedUnitAction(deprecations):

case DeprecatedUnitAction.RAISE:

raise UnitsError(msg)

case DeprecatedUnitAction.WARN:

warnings.warn(msg, UnitsWarning)

case DeprecatedUnitAction.CONVERT:

warnings.warn(

msg + " It cannot be automatically converted.", UnitsWarning

)

With the following defined elsewhere

DEPRECATED_UNIT_MSG: Final = "The unit {} has been deprecated in the {} standard."

I'm not sure deprecations = DeprecatedUnitAction(deprecations) is necessary given the type annotation, but the docstring made me put it in.

This is slightly more compact and avoids assigning to intermediate variables unless needed.

The creation of msg in separate branches may be an over-optimization.

First of all your suggestion would mean the deprecations value is checked whether or not there are any deprecated units, whereas with my code deprecations is ignored unless it is actually needed. That is a visible difference in code behavior.

For code readability it is best if things are defined close to where they are used unless doing so would cause code duplication. Unfortunately some of your suggestions move definitions away from usage while also increasing duplication. The claim that the suggested code is more compact is not true because shortening the function here by a few lines is achieved by adding even more lines elsewhere.

Given that it's a str enum, can't the parse step be moved anywhere?
Edit: I just did. I moved it to address your first point.
And doesn't _missing_ move things closer to their usage? It's like __contains__ and performs a similar separation of concerns between determining if an action is valid and performing that action. And Enums naturally contain a _missing_ check, which the proposed code doesn't use when raising on an incompatible input. This suggestion customizes _missing_ and uses the Enum mechanism.

I quite like the match here, as well as using the StrEnum (if we're using it at all) to generate the error message. I would not worry at all about performance in this path, where we're already dealing with a deprecated unit, so in an unlikely scenario.

I'd do CONVERT first, since then alternative is still close by.

mhvk

Thanks for doing this - I really like the overall idea.

My one doubt is about the enum -- I don't really see the benefit of it, especially because it is not used throughout (and I agree we cannot use it in user-facing parts). I would just stick with strings, which means fewer lines added too.

mhvk · 2025-09-12T18:21:57Z

+If automatic replacement is not possible then the deprecated unit is used for
+constructing the string and a warning is emitted.


Not the prettiest, but in principle one could do,

from astropy.units import cds cds.Crab.to_string(format="ogip")

mhvk · 2025-09-12T18:23:01Z

+If automatic replacement is not possible then the deprecated unit is used for
+constructing the string and a warning is emitted.


p.s. It is indeed a bit annoying that u.Unit("Crab", format="ogip", parse_strict="silent") actually is not silent...

mhvk · 2025-09-12T18:37:26Z

+        if s in cls._deprecated_units and deprecations != DeprecatedUnitAction.SILENT:
            alternative = (
                unit.represents if isinstance(unit := cls._units[s], Unit) else None
            )
            msg = f"The unit {s!r} has been deprecated in the {cls.__name__} standard."
            if alternative:
+                if deprecations == DeprecatedUnitAction.CONVERT:
+                    return alternative
                msg += f" Suggested: {cls.to_string(alternative)}."
-            warnings.warn(msg, UnitsWarning)
+
+            if deprecations == DeprecatedUnitAction.RAISE:
+                raise UnitsError(msg)
+            if deprecations == DeprecatedUnitAction.WARN:
+                warnings.warn(msg, UnitsWarning)
+            elif deprecations == DeprecatedUnitAction.CONVERT:
+                warnings.warn(
+                    msg + " It cannot be automatically converted.", UnitsWarning
+                )
+            else:
+                valid_options = ", ".join(map(repr, map(str, DeprecatedUnitAction)))
+                raise ValueError(
+                    f"invalid deprecation handling option: {deprecations!r}. Valid "
+                    f"options are {valid_options}."
+                )


I quite like the match here, as well as using the StrEnum (if we're using it at all) to generate the error message. I would not worry at all about performance in this path, where we're already dealing with a deprecated unit, so in an unlikely scenario.

I'd do CONVERT first, since then alternative is still close by.

eerovaher · 2025-09-15T14:47:33Z

My one doubt is about the enum -- I don't really see the benefit of it, especially because it is not used throughout (and I agree we cannot use it in user-facing parts). I would just stick with strings, which means fewer lines added too.

The docstrings of astropy.units.UnitBase.to_string() and astropy.units.format.Base.to_string() contain duplicated sections that describe deprecations. Using an enum allows us to avoid that duplication because the functions related to the enum can just refer to it. But making the enum public right now would raise the question why isn't the value of fraction an enum too (and parse_strict in the unit parsing functions). I have therefore avoided using the enum for deprecations publicly, but using it internally reduces the amount of code that would have to be updated when a public enum is introduced, and it also demonstrates that switching to a StrEnum publicly would not cause problems in practice for downstream code that is already using strings.

nstarman · 2025-09-15T18:41:52Z

+                        msg + " It cannot be automatically converted.", UnitsWarning
+                    )
+                case _:
+                    assert_never(deprecations)


nstarman

LGTM :)
@mhvk had a few points to address.

mhvk

Thanks, I like the match. As I write inline, I think it is a place where clarity is more important than performance, so would prefer to keep it simple. But if you prefer not to, that's fine too, so approving now.

mhvk · 2025-09-15T19:01:56Z

+    def _validate_unit(
+        cls, s: str, deprecations: DeprecatedUnitAction = DeprecatedUnitAction.WARN
+    ) -> UnitBase:
+        if (


I think the match is working out! But I'd still prefer to forego performance here and just start the match after the existing

if s in cls._deprecated_units: msg = "..." if (alternative := ...): msg += ... match DeprecatedUnitAction(deprecations): case DeprecatedUnitAction.CONVERT: if alternative: return alternative warnings.warn(msg + ...) case ...

It keeps the code easy to follow, with all logic to the various deprecated unit possibilities in one place; performance really is not a worry here.

eerovaher · 2025-09-15T22:06:57Z

It looks like the demonstration of StrEnum has convinced the reviewers that it is a good idea. We should create astropy/units/enums.py for the enums so that we could freely import them without having to worry about import loops.

The `"ogip"` and `"vounit"` formats have deprecated units. Converting such units to strings has so far caused an unavoidable warning. A new parameter of the `UnitBase.to_string()` and unit formatter `to_string()` methods allows silencing that warning or raising an error instead. This is analogous to the `parse_strict` parameter of unit parsing methods. The `StrEnum` that controls the new behavior is defined in a new module so that it could be imported without having to worry about import loops.

The "vounit" and "ogip" formats have deprecated units. It is now possible to instruct the unit formatters to try to replace deprecated units when writing them to strings. If automatic replacement is not possible the deprecated unit is used and a warning is emitted. An alternative would have been to implement a new method in `UnitBase` to replace deprecated units, which could be used like `u.erg.convert_deprecated("vounit").to_string(format="vounit")` but then the user would have to specify the unit format twice and such repetition should be avoided.

mhvk

Great, thanks for making that last change.

bsipocz · 2025-09-16T16:50:05Z

+
+If automatic replacement is not possible then the deprecated unit is used for
+constructing the string and a warning is emitted.
+As a last resort, it is possible to silence the warnings::


Late to this party, but calling this as the last resort is a bit overly judgy -- erg and Angstroms will likely stay with us for a while, no need for this snarky comment in the docs.

eerovaher added this to the v7.2.0 milestone Sep 2, 2025

eerovaher added units Enhancement labels Sep 2, 2025

github-actions Bot added the Docs label Sep 2, 2025

eerovaher commented Sep 5, 2025

View reviewed changes

Comment thread astropy/units/format/base.py Outdated

eerovaher force-pushed the writing-deprecated-units branch from 32ceac9 to f6b3199 Compare September 11, 2025 15:02

eerovaher commented Sep 11, 2025

View reviewed changes

eerovaher marked this pull request as ready for review September 11, 2025 15:29

eerovaher requested review from mhvk and nstarman September 11, 2025 15:29

nstarman reviewed Sep 11, 2025

View reviewed changes

mhvk reviewed Sep 12, 2025

View reviewed changes

eerovaher force-pushed the writing-deprecated-units branch from f6b3199 to d821e67 Compare September 15, 2025 14:21

nstarman reviewed Sep 15, 2025

View reviewed changes

nstarman approved these changes Sep 15, 2025

View reviewed changes

mhvk approved these changes Sep 15, 2025

View reviewed changes

eerovaher added 2 commits September 16, 2025 15:32

eerovaher force-pushed the writing-deprecated-units branch from d821e67 to 4088611 Compare September 16, 2025 13:39

eerovaher requested review from mhvk and nstarman September 16, 2025 14:14

nstarman approved these changes Sep 16, 2025

View reviewed changes

mhvk approved these changes Sep 16, 2025

View reviewed changes

mhvk merged commit 21b319f into astropy:main Sep 16, 2025
30 checks passed

bsipocz reviewed Sep 16, 2025

View reviewed changes

eerovaher deleted the writing-deprecated-units branch September 17, 2025 17:22

PreisTo added a commit to PreisTo/astromodels that referenced this pull request Dec 1, 2025

fix astropy 7.2.0 breaking change astropy/astropy#18586

b7199df

		If automatic replacement is not possible then the deprecated unit is used for
		constructing the string and a warning is emitted.

Conversation

eerovaher commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Sep 2, 2025

Uh oh!

github-actions Bot commented Sep 2, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eerovaher commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerovaher commented Sep 15, 2025

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

eerovaher commented Sep 2, 2025 •

edited

Loading

nstarman Sep 11, 2025 •

edited

Loading

nstarman Sep 12, 2025 •

edited

Loading

eerovaher commented Sep 15, 2025 •

edited

Loading