Repository navigation
Conversation
|
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.
|
| self.kwargs = [kwargs] if kwargs is not None else [{}] | ||
|
|
||
| def __add__(self, other): | ||
| def __add__(self, other: list[Any]) -> list[Any]: |
There was a problem hiding this comment.
This might not be the best annotation, but I couldn't come up with overloads that agree with list.__add__(), so I decided this is good enough.
| if TYPE_CHECKING: | ||
| from typing import TypeAlias | ||
|
|
||
| EquivalencyPair: TypeAlias = ( |
There was a problem hiding this comment.
This name is based on the phrasing used in our documentation. For example,
astropy/docs/units/equivalencies.rst
Line 16 in 92b38ae
astropy/astropy/units/equivalencies.py
Lines 107 to 109 in 92b38ae
astropy/astropy/units/equivalencies.py
Lines 598 to 600 in 92b38ae
nstarman
left a comment
There was a problem hiding this comment.
Looks good! I think there's one partial annotation.
mhvk
left a comment
There was a problem hiding this comment.
Looks good to me, except that the EquivalencyPair type seems too strict.
|
|
||
| EquivalencyPair: TypeAlias = ( | ||
| tuple[UnitBase, UnitBase | None] | ||
| | tuple[UnitBase, UnitBase, Callable[[npt.NDArray[Any]], npt.NDArray[Any]]] |
There was a problem hiding this comment.
Equivalency functions also take numbers, or really anything numeric. (A dask array would be fine.) With no guarantee that the same type is returned.
There was a problem hiding this comment.
My thinking was that equivalencies are used for converting the value of a Quantity, which is always an ndarray. Is that not the case?
There was a problem hiding this comment.
Mostly, it will be used in Quantity, indeed, so mostly it will be ndarray. But equivalencies can also be passed to unit.to and unit.get_converter, and then the callables are used more directly (well, via _condition_arg). See the first example on https://docs.astropy.org/en/latest/units/conversion.html but imaging an equivalency is used, is in,
u.one.to(u.arcsec, 1., equivalencies=u.dimensionless_angles())
# 206264.80624709636
Note that _condition_arg was refactored a short while ago to pass along anything with a .dtype, like a dask array. This was specifically to allow unit coversion with turning everything in ndarray.
There was a problem hiding this comment.
Mostly, it will be used in
Quantity, indeed, so mostly it will bendarray. But equivalencies can also be passed tounit.toandunit.get_converter, and then the callables are used more directly (well, via_condition_arg). See the first example on https://docs.astropy.org/en/latest/units/conversion.html but imaging an equivalency is used, is in,u.one.to(u.arcsec, 1., equivalencies=u.dimensionless_angles()) # 206264.80624709636
It should not be too difficult to replace npt.NDArray[Any] with npt.NDArray[Any] | UnitScale or npt.NDArray[Any] | UnitScaleLike.
Note that
_condition_argwas refactored a short while ago to pass along anything with a.dtype, like a dask array. This was specifically to allow unit coversion with turning everything inndarray.
Are you sure that this functionality works as intended with all the equivalencies? Have you tested it with logarithmic, parallax or doppler_redshift?
There was a problem hiding this comment.
What's the simplest solution for proceeding?
A HasShape temporary protocol?
There was a problem hiding this comment.
Just don't narrow Callable down for now and add a TODO?
|
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. |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 1 year, 6 months. I plan to close this in a month 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. |
Description
I am about to poke around in the unit equivalencies code, so it will be good to have annotations in place.