Recognize units written with unicode symbols also in composites by mhvk · Pull Request #17011 · astropy/astropy

mhvk · 2024-09-14T14:01:56Z

Units involving special unicode symbols can now be parsed also if they are
part of a composite unit, like "L☉/pc²".

Furthermore, a bug was fixed where "°C/s" was incorrectly interpreted as "°C/s^2".

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 · 2024-09-14T14:02:20Z

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.

mhvk

Some small clarifications to help review.

mhvk · 2024-09-14T14:02:38Z

astropy/units/format/generic.py

        elif not s.isascii():
            if s[0] == "\N{MICRO SIGN}":
                s = "u" + s[1:]
+            elif s[0] == "°":


With the change in what is recognized as units, conversion of degree sign can now be done here.

mhvk · 2024-09-14T14:04:48Z

astropy/units/format/generic.py


-    @classmethod
-    def _convert_deg(cls, m: Match[str]) -> str:
-        if len(m.string) == 1:


This was actually wrong, it should have been m.group(). Hence, for "°C/s", "°C" would be substituted with "deg_C/s" giving a full string of "deg_C/s/s".

mhvk · 2024-09-14T14:05:30Z

astropy/units/tests/test_format.py

        ("\N{GREEK SMALL LETTER MU}g", u.microgram),
        ("g\N{MINUS SIGN}1", u.g ** (-1)),
-        ("m\N{SUPERSCRIPT MINUS}\N{SUPERSCRIPT ONE}", 1 / u.m),
+        ("m\N{SUPERSCRIPT MINUS}\N{SUPERSCRIPT ONE}", u.m**-1),


Just ensuring we create a unit to compare with, not a Quantity.

mhvk · 2024-09-14T21:12:38Z

Now also includes a fix for odd CDS units (which end in 0). None very likely to be encountered, but it really brings out the risk of using multiple paths to determine what unit is intended.

eerovaher

All the other parsing formatters inherit _regex_deg and _convert_deg() from Generic, so removing them simplifies the other classes too.

eerovaher · 2024-09-16T17:15:30Z

astropy/units/format/generic.py

    @classmethod
    def _convert_superscript(cls, m: Match[str]) -> str:
        return f"({m.group().translate(cls._superscript_translations)})"


Do you think it would be a good idea to refactor the code in a follow-up pull request to get rid of this method too?

Not sure it can be captured as easily in an if statement, but in principle, yes. It would also be good to ensure mixes of superscript and regular digits do not work...

astropy/units/tests/test_format.py

eerovaher · 2024-09-16T17:42:48Z

docs/changes/units/17011.bugfix.rst

+Units involving special unicode symbols can now be parsed also if they are
+part of a composite unit, like "L☉/pc²".
+
+Similarly, CDS units ending in a 0 can now be part of composite units.
+
+Furthermore, a bug was fixed where "°C/s" was incorrectly interpreted as "°C/s^2".


I think users would appreciate if the common part of these three cases is summarized in a single sentence and the details are in an explicit list:

Suggested change

Units involving special unicode symbols can now be parsed also if they are

part of a composite unit, like "L☉/pc²".

Similarly, CDS units ending in a 0 can now be part of composite units.

Furthermore, a bug was fixed where "°C/s" was incorrectly interpreted as "°C/s^2".

The unit parsers are now much better at recognizing several unusual composite units:

- units involving special unicode symbols, like "L☉/pc²",

- units that include CDS units ending in a 0, like "eps0/s",

- units including "°C". For example, "°C/s" is no longer incorrectly interpreted as "°C/s^2".

Thanks, much better; used with slight modifications.

mhvk · 2024-09-16T18:15:09Z

OK, pushed the new version.

eerovaher · 2024-09-16T19:15:55Z

astropy/units/tests/test_format.py

+        (["eps0/mu0"], cds.eps0 / cds.mu0),
+        (["a0.s"], cds.a0 * u.s),


No harm in adding a comment here saying that these two are regression tests for #17010.

Not strictly true, just found it while investigating that. But more relevantly, this either means running all of CI again or an extra commit with ci skip - neither seems quite worth it to me, but happy to be told otherwise.

I'm not a units maintainer, @nstarman should do the final review.

I think in this case the git blame is good.
It would be good to find a way to move towards using Hypothesis, e.g. with these as @example

We're using hypothesis on Time, where I found it rather painful and noisy (though obviously it does help correctness, but just felt that we ended up having to increase the tolerance slightly many times...).

nstarman

This all looks good.
Also looking forward to continued simplifications by @eerovaher to make units/forma a little easier to grok 😆.

nstarman · 2024-09-17T20:20:05Z

astropy/units/format/cds.py

+        # are a few special ones (\h is Planch constant) and three that end in 0.
        def t_UNIT(t):
-            r"\%|°|\\h|((?!\d)\w)+"
+            r"%|°|\\h|(a|eps|mu)0|((?!\d)\w)+"


These are just documentation, right?

No! This docstring is used by the lexer as a regex to capture things that are to be considered units. Since !\d excludes digits, I had to put in the special case for a0, etc.

Ok! I recall this now from when I last looked through the formatting code in detail. I thought then as I think now that it's surprising to have docstrings with real code effects. It's not unreasonable in this circumstance, but perhaps a good refactor is to reformat this to be an instance of a class with a __call__ method.

class LexerFunc: regex: ... func: Callable[[T], R] __slots__ = ("regex", "func") def __call__(self, t): return self.func(t) def _t_Unit(t): t.value = cls._get_unit(t) return t t_Unit = LexerFunc(r"%|°|\\h|(a|eps|mu)0|((?!\d)\w)+", _t_Unit)

(units/format/ appears to be a good place to apply mypyc and significantly reduce python function overhead)

This is not to be done in this PR, of course!

(though we should probably move this to a separate Issue) @eerovaher, you've been upgrading units.format recently. Thoughts?

A separate issue may be a good idea - at the same time, we should also really update to the latest PLY - #13399

The requirement that the regexes must be in docstrings comes from PLY. Although we bundle it, it's still an external package, so we don't have full control over the API here.

nstarman · 2024-09-17T20:24:19Z

astropy/units/tests/test_format.py

+        (["eps0/mu0"], cds.eps0 / cds.mu0),
+        (["a0.s"], cds.a0 * u.s),


I think in this case the git blame is good.
It would be good to find a way to move towards using Hypothesis, e.g. with these as @example

mhvk · 2024-09-28T12:56:32Z

@nstarman - is this one good to go?

nstarman

lumberbot-app · 2024-09-29T20:44:45Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v6.1.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 2611b0be0535b5fce583ac7c15823964e504a185

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #17011: Recognize units written with unicode symbols also in composites'

Push to a named branch:

git push YOURFORK v6.1.x:auto-backport-of-pr-17011-on-v6.1.x

Create a PR against branch v6.1.x, I would have named this PR:

"Backport PR #17011 on branch v6.1.x (Recognize units written with unicode symbols also in composites)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

Recognize units written with unicode symbols also in composites (cherry picked from commit 2611b0b)

Backport PR #17011 on branch v6.1.x (Recognize units written with unicode symbols also in composites)

mhvk added units Bug backport-v6.1.x labels Sep 14, 2024

mhvk added this to the v6.1.4 milestone Sep 14, 2024

mhvk commented Sep 14, 2024

View reviewed changes

mhvk requested a review from eerovaher September 14, 2024 14:06

mhvk modified the milestones: v6.1.4, v7.0.0 Sep 14, 2024

mhvk force-pushed the units-composite-unicode branch from 3b3a693 to 5974f89 Compare September 14, 2024 21:11

This was referenced Sep 14, 2024

Refactoring of the parsing of simple units, moving short-circuit up #17012

Closed

Speed up parsing simple unit strings with Unit #17004

Merged

eerovaher reviewed Sep 16, 2024

View reviewed changes

mhvk added 5 commits September 16, 2024 14:12

TST: add tests for composite units involving unicode

91a80f0

BUG: ensure unicode units are recognized in composites as well

f44ca2b

TST: add tests for composite CDS units with 0 in their name

eea78b6

BUG: ensure CDS composite units work also for units with 0 in their name

301cc4c

DOC: changelog entry

467fa58

mhvk force-pushed the units-composite-unicode branch from 5974f89 to 467fa58 Compare September 16, 2024 18:14

eerovaher reviewed Sep 16, 2024

View reviewed changes

eerovaher requested a review from nstarman September 17, 2024 18:11

nstarman reviewed Sep 17, 2024

View reviewed changes

pllim modified the milestones: v6.1.4, v6.1.5 Sep 27, 2024

mhvk removed the backport-v6.1.x label Sep 28, 2024

mhvk added the backport-v6.1.x label Sep 28, 2024

nstarman approved these changes Sep 29, 2024

View reviewed changes

nstarman modified the milestones: v6.1.5, v7.0.0 Sep 29, 2024

nstarman merged commit 2611b0b into astropy:main Sep 29, 2024

lumberbot-app bot added the Still Needs Manual Backport label Sep 29, 2024

mhvk deleted the units-composite-unicode branch September 29, 2024 21:32

mhvk pushed a commit to mhvk/astropy that referenced this pull request Sep 29, 2024

Merge pull request astropy#17011 from mhvk/units-composite-unicode

e8e86ef

Recognize units written with unicode symbols also in composites (cherry picked from commit 2611b0b)

mhvk removed the Still Needs Manual Backport label Sep 29, 2024

pllim mentioned this pull request Sep 30, 2024

Backport PR #17011 on branch v6.1.x (Recognize units written with unicode symbols also in composites) #17091

Merged

1 task

pllim added a commit that referenced this pull request Sep 30, 2024

Merge pull request #17091 from mhvk/auto-backport-of-pr-17011-on-v6.1.x

a3c26c3

Backport PR #17011 on branch v6.1.x (Recognize units written with unicode symbols also in composites)

		(["eps0/mu0"], cds.eps0 / cds.mu0),
		(["a0.s"], cds.a0 * u.s),

Uh oh!

Conversation

mhvk commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 14, 2024

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!

mhvk commented Sep 14, 2024

Uh oh!

eerovaher 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 16, 2024

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!

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!

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!

mhvk commented Sep 28, 2024

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

lumberbot-app bot commented Sep 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mhvk commented Sep 14, 2024 •

edited

Loading