structured unit / quantity stuff

[nstarman]

Signed-off-by: Nathaniel Starkman (@nstarman) nstarkman@protonmail.com

Necessary for #13669

• Quantity now builds a structured unit if unit=None in the constructor and the value is structured.

TODO:

• changelog
• tests

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• 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 astropy-bot check might be missing; 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.

[github-actions]

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

[mhvk]

Yes, just changing the default would certainly seem fine!

A suggestion for making this easier, although that changes API slightly. But I think it is fine.

[mhvk]

Is there an associated version of python when this will no longer be needed?

[nstarman]

Yes. The nice thing is that pyupgrade in pre-commit should remove this when its obsolete and we bump the minimum version and run pre-commit on the repository.
Maybe that should be a step in the release process...

[eerovaher]

Is the line here just to avoid importing typing.Union?

[nstarman]

Currently, yes.

[eerovaher]

PEP 563 has been postponed indefinitely, so I wouldn't expect pyupgrade to remove the line in question any time soon. However, writing union types as X | Y instead of Union[X, Y] proposed by PEP 604 was adopted in Python 3.10, so if the import is only needed for that then it could be removed when support for Python 3.9 is dropped.

[nstarman]

Thanks for clearing up the timeline!

[mhvk]

Maybe mark with a comment # For python < 3.10.?

[nstarman]

✅

[mhvk]

I think for a private function we should be able to assume that the input is correct (i.e., just let the AttributeError or whatever else happen if not). python is not LBYL.

[nstarman]

I stuck this in for type narrowing since type hints are LBYL. There's no way to indicate that a dtype is structured except by checking. My editor is configured to check on type hints (Astropy makes it very unhappy). Hopefully NumPy will parametrize dtype as a Generic[...] or have a StructuredDtype protocol.

We could add that protocol...

@runtime_checkable
class StructuredDtype(Protocol):
    @property
    def names(self) -> tuple: ...
    @property
    def fields(self) -> Mapping: ...

[mhvk]

Understood, but can you nevertheless remove it? It is one thing to label things to help people and computers understand intent, quite another to make the code slower and more complex... I find it a beauty of python that one does not presume to know beforehand all possible input one might get.

[nstarman]

Probably more typing than Astropy wants right now, but the suggested Protocol should solve both problems. This function knows that the dtype is structured and there's no checks necessary.

[mhvk]

Yes, having that Protocol would indeed solve it. And could live in separate typing files, where it wouldn't bug me 😺 More seriously, something to suggest to numpy?

[nstarman]

✅

[nstarman]

Darn. https://docs.python.org/3/library/typing.html#typing.runtime_checkable

[nstarman]

Forgot it does not do ANY signature checking. The protocol will not work.

[mhvk]

That's a pity. But let's leave all of this out of this PR. As is, this private function is only called with well-defined input so no checks should be necessary.

[nstarman]

numpy/numpy#22286

[mhvk]

Looks all great. Approving since what remains is just a question of use of case.

[mhvk]

Rereading this, I wonder why you use all-caps here. I tended to treat those as "environment variables", i.e., the caps indicate that the variable is defined outside of the scope. But that is not the case here, so maybe one should use lower case?

[nstarman]

SGTM. I'll repush and merge on pass.