Add type hint annotations to astropy.units.physical#15914
Add type hint annotations to astropy.units.physical#15914namurphy wants to merge 1 commit intoastropy:mainfrom
astropy.units.physical#15914Conversation
|
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
| __all__ = ["def_physical_type", "get_physical_type", "PhysicalType"] | ||
|
|
||
| _units_and_physical_types = [ | ||
| _units_and_physical_types : list[tuple(u.UnitBase, str | set(str))] = [ |
There was a problem hiding this comment.
| _units_and_physical_types : list[tuple(u.UnitBase, str | set(str))] = [ | |
| _units_and_physical_types : list[tuple[u.UnitBase, str | set[str]]] = [ |
|
|
||
| @staticmethod | ||
| def _dimensionally_compatible_unit(obj): | ||
| def _dimensionally_compatible_unit(obj: object) -> u.UnitBase | NotImplemented: # ??? |
There was a problem hiding this comment.
this is a good place for typing.overload.
There was a problem hiding this comment.
Same comment as for __eq__ - I think we'd want the type information to tell what gives success, not what gives failure, where for this function failure means getting NotImplemented.
Note that I really worry about "this is a good place for typing.overload - if we are adding more than annotations, suddenly files can become like this
# Builtin types
@overload
def __new__(cls, dtype: type[builtins.bool], align: builtins.bool = ..., copy: builtins.bool = ..., metadata: dict[builtins.str, Any] = ...) -> dtype[np.bool]: ...
@overload
def __new__(cls, dtype: type[int], align: builtins.bool = ..., copy: builtins.bool = ..., metadata: dict[builtins.str, Any] = ...) -> dtype[int_]: ...
@overload
def __new__(cls, dtype: None | type[float], align: builtins.bool = ..., copy: builtins.bool = ..., metadata: dict[builtins.str, Any] = ...) -> dtype[float64]: ...
@overload
def __new__(cls, dtype: type[complex], align: builtins.bool = ..., copy: builtins.bool = ..., metadata: dict[builtins.str, Any] = ...) -> dtype[complex128]: ...
@overload
def __new__(cls, dtype: type[builtins.str], align: builtins.bool = ..., copy: builtins.bool = ..., metadata: dict[builtins.str, Any] = ...) -> dtype[str_]: ...
@overload
def __new__(cls, dtype: type[bytes], align: builtins.bool = ..., copy: builtins.bool = ..., metadata: dict[builtins.str, Any] = ...) -> dtype[bytes_]: ...
Can we pretty please agree that overload has no place in regular python code files?
| return new_unit.physical_type | ||
|
|
||
| def __mul__(self, other): | ||
| def __mul__(self, other: object) -> Self | NotImplemented: |
There was a problem hiding this comment.
The standard is actually to not put NotImplemented into the type hint of dunder methods.
There was a problem hiding this comment.
Bit of a flyby, as I was just looking how the typing "looks": these ones seem rather superfluous; are there not some standard defaults for dunder operations like these?
There was a problem hiding this comment.
@mhvk — that is a great question that I do not know the answer to! I think that static type checkers like mypy still need type hints in order to check dunder methods, but I'm not sure.
When adding type hints to plasmapy.particles, I was able to use autotyping to automatically add type hints for dunder methods, which was pretty straightforward.
There was a problem hiding this comment.
See comment about __eq__ - it seems much more logical to me to put here things that allow __mul__ to succeed.
| yield from self._physical_type_list | ||
|
|
||
| def __eq__(self, other): | ||
| def __eq__(self, other: object) -> bool: |
There was a problem hiding this comment.
Shouldn't this be
| def __eq__(self, other: object) -> bool: | |
| def __eq__(self, other: PhysicalType | str) -> bool: |
There was a problem hiding this comment.
It's valid to do PhysicalType() == 4, which will return False after trying 4.__req__().
There was a problem hiding this comment.
Unresolving this, since I think @eerovaher's comment is actually relevant: isn't typing supposed to tell what is needed to make this function succeed? With PhysicalType|str, that is clear, with object you get zero information.
I guess for these dunder methods, it depends on what you mean by "succeed" - I'd argue that returning NotImplemented is a form of not succeeding, since if the reverse also does it, you get a TypeError. More generally, that the reverse is called is irrelevant for thinking about success of this function.
There was a problem hiding this comment.
See https://discuss.python.org/t/make-type-hints-for-eq-of-primitives-less-strict/34240/10 for a discussion where people are advocating for making typing dunder methods less strict. The typing of most dunder methods is limited by inheritance from "object". Currently the correct annotation for equality is __eq__(self, other: object) -> bool. Not annotating the return type of NotImplemented is problematic but baked into Python ATM. Accepting any input is correct since evaluating an equality to False is a valid output, even if it happens from a double NotImplemented behind-the-scenes.
There was a problem hiding this comment.
Having other be restricted only makes sense if the method errors or doesn't work on that input. __eq__ et al work, returning NotImplemented / False.
There was a problem hiding this comment.
I understand the logic, but it is not a useful one to humans or much in the spirit of what NotImplemented means - I'd say we don't annotate these until the discussion has concluded.
| # standard physical types defined here. | ||
| if __doc__ is not None: | ||
| doclines = [ | ||
| doclines: list[str] = [ |
There was a problem hiding this comment.
Why does this need an annotation?
|
|
||
|
|
||
| def get_physical_type(obj): | ||
| def get_physical_type(obj: object) -> PhysicalType: |
There was a problem hiding this comment.
Seems good to have QuantityLike and PhysicalTypeLike defined, use it here.
There was a problem hiding this comment.
👍 on using a type alias PhysicalTypeLike. This is again driven by use of type annotations as developer docs, not trying to please mypy. Whether PhysicalTypeLike tries to enumerate all the possibilities or if it is just an empty type alias makes no difference to me, but seeing that while developing code is very useful.
There was a problem hiding this comment.
Indeed, and I like that those can be stored in a types.py file, so one can fiddle with those definitions without having to change how the code itself looks.
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
Description
This pull request adds type hint annotations to
astropy.units.physical.It's only a first draft so far since I haven't checked it with mypy at all. I may add some type hints to the corresponding tests too while I'm at it.
It'd be fine to squash this PR.