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.

ENCHMARKS NOT SIGNIFICANTLY CHANGED. 🤷‍♀️

BENCHMARKS NOT SIGNIFICANTLY CHANGED. 🤷‍♀️

The benefit is not as obvious here - it is a slight speed-up, but mostly is there to ensure that #16892 will not slow things down.

I don't know what the definition of "significant" is in our benchmarks, but if you dig through the logs you will see that the difference between the relevant timings is much larger than their uncertainty, i.e. the change is significant:

https://github.com/astropy/astropy/actions/runs/10836066679/job/30068962593?pr=17004#step:7:636

https://github.com/astropy/astropy/actions/runs/10836066679/job/30068962593?pr=17004#step:7:1143

Replying to the suggestions by @mhvk

            # seems more logical to switch order
            if isinstance(s, bytes):
                s = s.decode("ascii")

There was no significant speedup from placing this check after the shortcut.

            # might as well speed this up too.
            f = unit_format.Generic if format is None else unit_format.get_format(format)

Changing this part had no noticeable effect on performance, but getting rid of the redundant if format is None check does simplify the code.

            # For Generic, try to short-circuit simple units.
            if (f is unit_format.Generic
                    and (unit := get_current_unit_registry().registry.get(s))):
                return unit

My updates speed up parsing with all the astropy unit formatters. Trying to limit this shortcut to Generic will slow down the others because they would no longer benefit from the shortcut that they currently inherit from Generic._do_parse(). We could avoid that slowdown by keeping the shortcut also in _do_parse(), but that makes the code more complex and slows down parsing composite units, because they would try and fail to take the shortcut twice.

The main benefit I saw of my suggestion is that it does not rely on private methods at all. But it should also be (marginally) faster for the by-far most common case of format=None, both for simple units and for complex ones (latter because of not having to deal with a failed try/except). I don't think there is much need to worry about parsing in other formats - that is almost exclusively done during I/O where the reading of associated data sets dominates.

Re: #17004 (comment) -- Maybe @MridulS knows since he implemented the infrastructure.

The following patch causes multiple test failures:

--- a/astropy/units/core.py
+++ b/astropy/units/core.py
@@ -2091,12 +2091,10 @@ class _UnitMetaClass(type):
             if isinstance(s, bytes):
                 s = s.decode("ascii")
 
-            try:
-                return f._parse_unit(s, detailed_exception=False)  # Try a shortcut
-            except (AttributeError, ValueError):
-                # No `f._parse_unit()` (AttributeError)
-                # or `s` was a composite unit or had weird Unicode in it (ValueError).
-                pass
+            if f is unit_format.Generic and (
+                unit := get_current_unit_registry().registry.get(s)
+            ):
+                return unit
 
             try:
                 return f.parse(s)

That's weird, I tested it yesterday. Will check.

The following patch causes multiple test failures:

The reason is that your other change is interfering; if starting from your PR, I remove the try/except you added in core.py, one also gets failures.

My original suggestion was to move a shortcut from Generic._do_parse() to _UnitMetaClass. Are you suggesting we should keep the current shortcut in Generic._do_parse() and implement an additional new shortcut in _UnitMetaClass?

I'm actually not sure what is the best way right now. Certainly, the failure I mentioned above shows that current implementation is a bit broken - see #17010. I'll see if I can find some time over the weekend to solve that - that may also make clear what the best path forward is.

I found that removing the short-circuit showed quite a few bugs - those also show up if one tries to create composite units, and are fixed in #17011. I think this in itself makes the case stronger not to try too much short-circuiting in the parsers - they should just parse! So, I had a trial for moving it up in #17012.