add unit-aware quantity annotations.

[nstarman]

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

Description

A __class_getitem__ method will be added to Quantity to shortcut creating unit aware annotations.
Quantity[<unit>, other_annotations] == Annotated[Quantity, <unit>, other_annotation].
As a class method, this will similarly work for all subclasses of Quantity.

The annotations used by quantity_input, like u.km or distance, are not compliant with static typing since the inputs are of (sub)type Quantity, but the annotations are not. This problem can be solved by the typing.Annotated class, from this PEP which allows valid static type annotations to include metadata. Here we implement the machinery to allow for unit-aware type annotations.

However, until astropy supports the official backport typing_extensions or the minimum python version 3.9+, metadata cannot be associated with typing. So to be compatible with quantity_input in this case a Quantity[unit] just returns the unit.

Part Of #10655

[pllim]

Maybe this is a little too ahead of its time right now. We should wait for astropy to drop Python 3.6 first before proceeding here, to keep things simple. Hopefully for 5.0?

[nstarman]

What about the other parts of the PR? The __class_getitem__ is only introduced as a convenience method for creating unit-aware annotations as Annotated[type, <unit>] a la this PEP.
I've heard from @mhvk, who likes the idea.
Again, this idea is forward thinking since Annotated becomes a built-in static type in py3.9+. If we add the backport as a dependency then it would work on py3.5+. Until either happens we can emulate the API.

[mhvk]

@nstarman - my sense would indeed be to make the Quantity[unit] part something that only works on python >=3.7; I think we can make sure that it does not affect the documented behaviour of quantity_input, and, as you note, we can simply not run the relevant tests on older python.

p.s. Barely looked at the actual code. General question: should our type be a sub-type of ndarray? (if there is such a thing).

[nstarman]

General question: should our type be a sub-type of ndarray? (if there is such a thing).

From what I've read, static typing with class annotations follows the expected MRO. I believe this means that Annotated[Quantity, <unit>] is automatically a sub-type of ndarray and supertype to Angle, Distance, etc. Static or run-time typecheckers would permit subclasses of Quantity, but not an ndarray, to be passed as input.

I definitely need to do some checking on covariance / contravariance of subclasses, but I don't think this will be an issue.

I think the relevant points are mentioned in https://docs.python.org/3/library/typing.html#typing.Type

[nstarman]

I'm trying to find a good place to put the utility function _parse_allowed_type_hint (currently in units.quantity). Really, the function tries to make the input a Unit instance and checks if it's a valid physical type if it cannot. It's quite similar to _get_allowed_units implemented elsewhere in units. Perhaps they can be combined & maybe made public?

def _parse_allowed_type_hint(arg, detailed_exception=True):
    try:  # unit passed in as a string or unit or Quantity
        unit = Unit(arg)

    except ValueError:  # physical type or mistake
        from astropy.units.physical import _unit_physical_mapping  # TODO move import
        if arg in _unit_physical_mapping:  # valid physical type
            unit = arg  # pass through unchanged
        else:   # Function argument arg is invalid
            if detailed_exception:
                raise ValueError(
                    (
                        f"Invalid unit or physical type '{arg}'.\n"
                        f"Did you mean: {'TODO!'}"
                        # TODO did_you_mean(arg, candidates)
                    )
                )
            else:
                raise ValueError(f"Invalid unit or physical type '{arg}'.")

    return unit

[pep8speaks]

Hello @nstarman 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-10-29 20:38:52 UTC

[adrn]

Just a note to say that I plan to take a look at this by the end of this week - thanks for the idea here, @nstarman !

[nstarman]

The code now compiles and has unit testing. If we want to go that route, it currently implements a version check for py3.7+ to add the __class_getitem__ method to Quantity and Annotated implements a metaclass for py3.6 compatibility to the dunder method.
I'm going to add documentation warning that Annotated is a stand-in for typing.Annotated and should be used accordingly.

Also, quantity_input needs modification has been modified to accept these annotations. This will work with both the stand-in Annotated and actual static type.

Also, though this PR skirts around the actual introduction of static typing, it comes close. I've put the code for Annotated in units.typing, but this might be entirely the wrong place for it. Is astropy planning to have a typing submodule inside every module like units and constants or one centralized location? Is there any consensus on how / where static typing should / will be implemented?

[nstarman]

@taldcroft This might still be a WIP since #10689 would obviate the need for an Annotated workaround, but all tests are currently passing.

[pllim]

Hello. I converted your PR to a draft PR due to #10706 .

[nstarman]

@mhvk I finished implementing how static typing can work with single annotations. If you have time, I want to consult on the convention for multiple annotations.
As I see it, there are two options:

1. Quantity[u.m, u.km, "time"] is allowed and produces Annotated[u.Quantity, (u.m, u.km, "time")]
2. or it isn't allowed. To get multiple unit-aware annotations Quantity[] must be called multiple times.

I'm strongly inclined towards the latter for 2 reasons:

1. Attaching non-unit metadata is much clearer
   Quantity[u.m, "a non-unit annotation", "and another"]
2. There is only one possible convention for multiple annotations to a function, which is:
   T.Union[Quantity[u.m], Quantity["time"], float, None]
   ( produces T.Union[Annotated[Quantity, u.m], Annotated[Quantity, "time"], float, NoneType])

What do you think?

[mhvk]

@nstarman - on your possibilities, I agree with your option 2, i.e., not allowing multiple units. Partially, this is because I could imagine that we'll need to merge some of the numpy static annotation in there as well, i.e., things like Quantity[u.cycle, np.dtype[complex]] (well, not the best example...).

If we do allow multiple units, they would definitely need to be their own tuple - but I think we can always deal with that as an extension.

[nstarman]

Quantity[u.cycle, np.dtype[complex]]

That's a use case I hadn't considered. What if we wanted to annotate something as a complex Quantity without specifying a unit?

Right now Quantity[np.dtype[complex]] isn't allowed since we do a unit check on the 1st annotation.

If we want to force the 1st-is-unit convention, then we'd need a null unit as the first argument. Quantity[u.AnyUnit, np.dtype[complex]] or Quantity[None, np.dtype[complex]] or Quantity[typing.Any, np.dtype[complex]] or something similar.
Or we don't do the unit check?
Or an unspecified option 3.

[mhvk]

Really nice! I'm happy with the simplifications. A few nitpicky remaining comments...

[mhvk]

Not super important, but perhaps good to check this warning is actually raised.

[nstarman]

I know this was important, but I don't remember why, so it's a little hard to design a test! 🤦

[mhvk]

Might it then be best to just remove it? If no tests fail, it cannot have been that important!

[nstarman]

Done!

[namurphy]

Just wanted to drop in and say that I'm excited for this PR to be merged! Thank you for working on it!

[mhvk]

Really only nitpicks left, with the largest one a suggested different import for a numpy version check. I'll approve so you can merge once those are addressed.

This is very nice!! So, probably another little TODO is to add to What's new next week!

[mhvk]

Might it then be best to just remove it? If no tests fail, it cannot have been that important!

[mhvk]

Maybe simplify: "... is installed; or an ..." and then delete "else-wise, depending on the input."

[nstarman]

I just made it a bullet point list.

[mhvk]

Should this just return a? If I understood the code right, the return annotation will actually enforce the unit, so the test would still pass.

[nstarman]

@mhvk

About this... I personally prefer if the code just validated the value, not enforced units, but I didn't want to change the behavior.
Part of the reason I prefer just validation is consistency with the behavior for the inputs. Also the quantity_input refactor is going to enable unit enforcement on inputs and outputs in a subclass decorator. Also fine-grained user control on a per-input / output basis.

Thoughts?

[mhvk]

Agreed that -> Quantity['length'] should be possible too, and that quantity_input should allow more control:

1. Check quantities of right physical type
2. Convert input/output to specific units
3. As 2, but drop to array, and attach unit on output (Decorator for converting quantity to arrays in specific units? #7368)

For now, fine to leave the test as is!

[nstarman]

SGTM. Something to consider for #10655

[mhvk]

@nstarman - any specific things I should look at? I had approved already, so am in principle quite happy to just leave it to you.

[nstarman]

Thanks @mhvk. I think it's ready to merge! I'm looking forward to #10655.

[WilliamJamieson]

Looks like this PR is breaking #11942 and #11634. See: https://github.com/astropy/astropy/runs/4051645176?check_suite_focus=true

[nstarman]

Yeah. I'm working on a fix now. Sigh. Should have rebased on your fix before merging this. Passed all my tests in py3.9

[nstarman]

Odd. Made a new venv locally with all same versions. Not getting these errors.
On to lengthy tox runs with pytest —pdb option.

[dhomeier]

It's passing on 3.9 and 3.10, but failing with Python 3.8.

[dhomeier]

Yes, the broken 3.10 test did cause most other tests to be cancelled before, including the 3.8 ones...

[dhomeier]

Odd. Made a new venv locally with all same versions. Not getting these errors.
On to lengthy tox runs with pytest —pdb option.

What platform is that? Having the same failures with

Platform: macOS-10.14.6-x86_64-i386-64bit

Executable: /opt/bin/python3.8

Full Python Version: 
3.8.11 (default, Jul 19 2021, 15:45:25) 
[Clang 11.0.0 (clang-1100.0.33.17)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions: 
Numpy: 1.21.2
Scipy: 1.7.1
Matplotlib: 3.4.2
h5py: 2.10.0
Pandas: 1.2.4
PyERFA: 2.0.0
Cython: 0.29.24
Scikit-image: not available
asdf: 2.8.1

Perhaps as a quick fix just mark them skip for 3.8? Keeps all open PRs from running the complete test suite otherwise.

[nstarman]

SGTM. I'll push two fixes. The skip and and actual fix.