Repository navigation
Compound units are not valid type annotations #14134
Description
Description
When type hinting with units, mypy errors on any unit that is 'compound'.
Expected behavior
Compound units are a valid type hint.
Actual behavior
mypy errors with
type_hint.py:9: error: Invalid type comment or annotation
Found 1 error in 1 file (checked 1 source file)
Steps to Reproduce
- Save code below to a file
mypy test.py
import astropy.units as u
import numpy as np
# This annotation is fine
def square_area(size: u.m):
return size**2
# This annotation is not valid
def square_size(area: u.m**2):
return np.sqrt(area)System Details
macOS-12.6.1-arm64-arm-64bit
Python 3.10.8 | packaged by conda-forge | (main, Nov 22 2022, 08:25:29) [Clang 14.0.6 ]
Numpy 1.23.4
pyerfa 2.0.0.1
astropy 5.1.1
Scipy 1.9.3
Matplotlib 3.6.2
mypy 0.982 (compiled: yes)
Activity
pllim commentedon Dec 7, 2022
I don't think astropy ever promised to be 100% compatible with typing yet? Maybe @nstarman remembers.
dstansby commentedon Dec 7, 2022
Maybe this is a feature request then 😉 - even if astropy doesn't 100% work with typing, it would be good for unit type hints to at least not error with
mypy.eerovaher commentedon Dec 7, 2022
Typing was discussed at the Coordination meeting, and in short the conclusion was that typing deserves an APE. There is no point in opening any issues about typing before such an APE is accepted.
Cadair commentedon Dec 7, 2022
To be fair, in my opinion there is a big difference between Astropy using typing (needs an APE) and astropy supporting downstream packages using Astropy classes as type hints (probably doesn't need an APE)
dstansby commentedon Dec 7, 2022
Well I don't feel very welcomed by this... how am I meant to know what to report and what not to bother spending time making a small reproducible example and reporting?
mhvk commentedon Dec 7, 2022
@dstansby - indeed, how could you know! Sorry!
Right now, for units, the idea we're trying to move to (led by @nstarman) is that we would type something like what you have with
Quantity[u.m**2]-- which I think is cleaner since it makes clear that a Quantity with area units is expected. I'm actually not quite sure whether this already works. Of course, this would then also work forquantity_input.nstarman commentedon Dec 7, 2022
Yes, that's the intended direction.
Our current problem is that for this to be a proper type annotation we would need to make a Mypy plugin to treat instances of
Unitas the class itself and attach the unit instance as metadata (in the form of anAnnotation) to the Quantity type. Also, we want variadic typing, which likewise requires a custom mypy plugin since variadic types are not supported out of the box. We could do it all, I think I know how, but the scale of the problem, requiring an APE, and the time commitment on my end have thus far proven too large.Also, this shouldn't be valid since an annotation with the unit should only ever be valid if the parameter is a subclass of that unit.
Typing, except in the cases of Literals, works with classes, not instances. I think if mypy isn't complaining then it's because the annotation is being treated as a string (if
from __future__ import annotations) or the mypy settings aren't strict enough.If the following is broken, I can take a look at a bug fix.
dstansby commentedon Dec 7, 2022
Thanks for suggesting the
u.Quantity[u.m]sytnax, I hadn't come across it before. Do you think it would be worth updating the docs to use this syntax instead of justu.mso users are exposed to this way of typing that plays nicely withmypy? For example, in the examples at https://docs.astropy.org/en/stable/api/astropy.units.quantity_input.html the typing is using the unit instance and notu.Quantity.mhvk commentedon Dec 7, 2022
Yes, we got stopped a bit in the middle, but indeed there is no reason not to start using the way -- or at least describe it!
Cadair commentedon Dec 8, 2022
That's because it's not using "typing" at all, it's using the function annotation syntax in the language for something other than typing 😉
That's not to say it shouldn't all be documented, it definitely should. I just like being a pendant and reminding everyone that it's perfectly valid to use annotations for things which aren't typing.
Cadair commentedon Dec 8, 2022
When you say this what do you mean? Do you mean MyPy wouldn't work with
u.Quantity[u.m]or withu.mitself or something else?Cadair commentedon Dec 8, 2022
To bring this up again, as I am not sure where else to talk about it. What do we think needs an APE exactly?
dstansby commentedon Dec 8, 2022
I've just checked, and
mypyis happy with theu.Quantitytyping. If anyone else finds this issue and wants a solution, the following works:So I'm personally happy for this issue to be closed? I think it would be nice to replace the function annotations in the astropy docs with this syntax, so users get used to using annotations that are compatible with
mypy, but appreciate that's a big job (but would be happy to chip away at it myself)dstansby commentedon Dec 8, 2022
re.
mypyandastropy, a great first and self-contained task might be to getmypypassing on theastropycode base without adding any additional typing. There was a bunch of errors coming fromastropywhen I ran the above code without--follow-imports=skipmhvk commentedon Dec 8, 2022
I think the quantity annotation is somewhat independent of typing, just providing what a clearer way to do the same thing for
quantity_input. A short update to the docs that explains is would seem a good idea.mhvk commentedon Dec 8, 2022
On a possible APE, I think that is necessary also to discuss how to add typing. There is a lot to be said to go the numpy route and add stub files, so that those who do not want to bother having to know it, or feel that it goes against the beauty of duck typing, never have to deal with it.