Avoid resorting units when converting to strings - #14439

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 "When to rebase and squash commits".
• 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.

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

Well, with one approval, I'll open this for general review! Thanks, @nstarman!

Rebased this PR that removes extra sorting when units are represented as strings with unit.to_string(). In general, this seems to improve the readability of the units (especially FITS and VOUNIT), but it implies a lot of small changes (and will probably cause downstream doctests to fail). Two questions:

1. Does this need a what's new entry?
2. I wondered a bit whether one could go for a "standard" representation for anything for which we have a physical type (e.g., "erg/(s cm2 A)" rather than "erg/(A s cm2)" as it is now). Not totally trivial since the physical types do not store how their unit is represented currently. Overall, I felt this might be a good idea but is more work than I have time for now. Of course, it is supplementary to this PR, so can be done after. But ideally we change the format very rarely, so if we merge the present PR, further changes should preferably happen before 5.3 is released.

1. Does this need a what's new entry?

I think so, esp if point 2 isn't completed by v5.3!

I wondered a bit whether one could go for a "standard" representation for anything for which we have a physical type (e.g., "erg/(s cm2 A)" rather than "erg/(A s cm2)" as it is now).

I think that would be fantastic!

Thanks! I've added a what's-new entry (and rebased for good measure). It is probably not a bad idea to merge this well before the feature freeze date, so we can get some feedback (e.g., if many doctests break). That also gives some time to experiment with possibly using a default representation based on physical type.

Need to fix doc warning:

docs/whatsnew/5.3.rst:156: WARNING: Duplicate explicit target name: "whatsnew-5.3-unit-formats-fraction".

I don't know. I too agree that this PR is good improvement. I need to sleep on this over the weekend.

What about the UNIT_CMP flag that can be turned on globally. It wouldn't require Astropy to be installed since it would be a regex to find <Quantity and then parse to find the unit bit and then rearrange based on the new rules. Ugh sounds like a fair bit of work, but I think it would solve all pain points except writing it.

One possible issue is that the unit is not only printed as part of <Quantity ...> -- think table columns (or the perhaps the second example listed above).

Aside, could one use something like .. doctest-requires : astropy>=5.3? Or can one not use version information?

.. doctest-requires : astropy>=5.3

Unfortunately, no... scientific-python/pytest-doctestplus#193

But somehow you can do it via doctest_subpackage_requires which isn't as flexible but might be better than nothing.

You mean this? scientific-python/pytest-doctestplus#112

Actually, it may be a reasonable solution to tell people that they can set the style with a version constraint in setup.cfg (if I understood it correctly).

Yes, but alas you cannot just ask doctestplus to only require certain astropy version for that one line, it is whole module. Not ideal but might be the best we can do for now.

I don't think that is that bad -- it shows that examples work with the most recent version of astropy; presumably, the tests cover the rests.

If you are curious on how I am handling this downstream, please see the following PRs and feel free to comment on them if you have better ideas. Thanks!

• TST: Unit str compatibility with astropy 5.3 spacetelescope/jdaviz#2077
• DOC: Use astropy<5.3 for doctest in spectral_cube.rst and analysis.rst specutils#1034

Hey, so one can do __doctest_requires__ = {"BlackBody": ["astropy<5.3"]}? That's pretty nice at least for the python files! And I think the .rst files being covered in setup.cfg is not that bad either.

So, I guess the to-do item may be to have this as part of either the what's-new or the API change fragment...

Might be easier to explain in What's New. Do you want to do a follow-up PR? If not, I can take a stab when I have time.

Makes sense. If you can have a stab, that would be great - I'll be mostly out of action until term ends in April...

Please see #14526

This bit astroquery as well, and the suggested workaround in setup.cfg is not exactly nice (we do want to test both old and new astropy versions).

@bsipocz - sorry to hear that this bit you... I love the principle of doctests, of ensuring that what the user will see when they run some example is actually what is in the documentation, but it does seem they raise the barrier rather a lot for making changes to how output looks - one shouldn't have to think of formatting as part of the API!

OTOH, if formatting is not important enough to consider backward compatibility, it may not be worth changing it and causing such widespread issues downstream, at least not without also adding tools that provide reasonable workarounds (globally skipping doctesting in rst files is not really a nice workaround).

It's a hard one - the first FITS unit of beam^-1 Jy really had to go; I guess we could have left it at that and minimized the pain by not changing generic as well.

On balance, we felt it was worth it. It is a bit like numpy in 1.13 getting around to finally clean-up the number of significant digits that get included in output (following python doing the same). Output looks much better now but it was a gigantic pain (even with FLOAT_CMP; we thought about an equivalent for units, but it is just impossible).

Unit string changes will break ASDF unit validation as jsonschema only exactly matches the listed unit string. Resulting in the need for spacetelescope/roman_datamodels#258. I think this maybe the only project currently using ASDF's unit support, so this should not be a big issue right now. In the future, please tag me on any unit string changes.

@WilliamJamieson , you can always add yourself to the "units" section in codeowners, then you will get pinged automatically. FYI.

@WilliamJamieson - I think this is going to be the last string change in a very long time.... Hopefully mostly because it is now as "right" as it can be, but certainly also to avoid again causing breakage and thus loss of time that could be better spent elsewhere...