Skip to content

Make several argument defaults immutable (and annotate them) #17496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mhvk merged 2 commits into from
Dec 4, 2024
Merged

Make several argument defaults immutable (and annotate them) #17496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57b15ce
Add more type annotations to `units`
eerovaher Dec 4, 2024
cd2604e
Make several function argument defaults immutable
eerovaher Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions astropy/units/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import warnings
from functools import cached_property
from threading import RLock
from typing import TYPE_CHECKING
from typing import TYPE_CHECKING, overload

import numpy as np

Expand All @@ -32,13 +32,13 @@
)

if TYPE_CHECKING:
from collections.abc import Collection, Iterable, Mapping, MutableMapping
from collections.abc import Collection, Iterable, Mapping, MutableMapping, Sequence
from types import TracebackType
from typing import Any, Final, Literal, Self

from .physical import PhysicalType
from .quantity import Quantity
from .typing import Real, UnitPower, UnitScale
from .typing import Complex, Real, UnitPower, UnitScale

__all__ = [
"CompositeUnit",
Expand Down Expand Up @@ -1211,7 +1211,7 @@ def in_units(self, other, value=1.0, equivalencies=[]):
"""
return self.to(other, value=value, equivalencies=equivalencies)

def decompose(self, bases: Collection[UnitBase] = set()) -> UnitBase:
def decompose(self, bases: Collection[UnitBase] = ()) -> UnitBase:
"""
Return a unit object composed of only irreducible units.

Expand Down Expand Up @@ -1901,7 +1901,7 @@ def represents(self) -> Self:
"""
return self

def decompose(self, bases: Collection[UnitBase] = set()) -> UnitBase:
def decompose(self, bases: Collection[UnitBase] = ()) -> UnitBase:
if len(bases) and self not in bases:
for base in bases:
try:
Expand Down Expand Up @@ -2201,7 +2201,7 @@ def represents(self) -> UnitBase:
"""The unit that this named unit represents."""
return self._represents

def decompose(self, bases: Collection[UnitBase] = set()) -> UnitBase:
def decompose(self, bases: Collection[UnitBase] = ()) -> UnitBase:
return self._represents.decompose(bases=bases)

def is_unity(self) -> bool:
Expand Down Expand Up @@ -2262,13 +2262,36 @@ class CompositeUnit(UnitBase):

_decomposed_cache: CompositeUnit | None = None

# _error_check can switch off runtime validation of scale, bases and powers.
# These overloads enable type checkers to validate statically.
@overload
def __init__(
self,
scale: Complex,
bases: Sequence[UnitBase],
powers: Sequence[Real],
decompose: bool = False,
decompose_bases: Collection[UnitBase] = (),
_error_check: Literal[True] = True,
) -> None: ...
@overload
def __init__(
self,
scale: UnitScale,
bases: Sequence[UnitBase],
powers: Sequence[UnitPower],
decompose: bool = False,
decompose_bases: Collection[UnitBase] = (),
_error_check: Literal[False] = False,
) -> None: ...

def __init__(
self,
scale,
bases,
powers,
decompose=False,
decompose_bases=set(),
decompose_bases=(),
_error_check=True,
):
# There are many cases internal to astropy.units where we
Expand Down Expand Up @@ -2338,7 +2361,9 @@ def powers(self) -> list[UnitPower]:
"""The powers of the bases of the composite unit."""
return self._powers

def _expand_and_gather(self, decompose=False, bases=set()):
def _expand_and_gather(
self, decompose: bool = False, bases: Collection[UnitBase] = ()
):
def add_unit(unit, power, scale):
if bases and unit not in bases:
for base in bases:
Expand Down Expand Up @@ -2382,7 +2407,7 @@ def add_unit(unit, power, scale):
def __copy__(self) -> CompositeUnit:
return CompositeUnit(self._scale, self._bases[:], self._powers[:])

def decompose(self, bases: Collection[UnitBase] = set()) -> CompositeUnit:
def decompose(self, bases: Collection[UnitBase] = ()) -> CompositeUnit:
if len(bases) == 0 and self._decomposed_cache is not None:
return self._decomposed_cache

Expand Down
7 changes: 5 additions & 2 deletions astropy/units/function/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
from numpy._core import umath as np_umath

if TYPE_CHECKING:
from collections.abc import Collection
from typing import Self

from astropy.units.typing import UnitPower

__all__ = ["FunctionQuantity", "FunctionUnitBase"]
Expand Down Expand Up @@ -174,7 +177,7 @@ def equivalencies(self):
return [(self, self.physical_unit, self.to_physical, self.from_physical)]

# ↓↓↓ properties/methods required to behave like a unit
def decompose(self, bases=set()):
def decompose(self, bases: Collection[UnitBase] = ()) -> Self:
"""Copy the current unit with the physical unit decomposed.

For details, see `~astropy.units.UnitBase.decompose`.
Expand Down Expand Up @@ -627,7 +630,7 @@ def cgs(self):
"""Return a copy with the physical unit in CGS units."""
return self.__class__(self.physical.cgs)

def decompose(self, bases=[]):
def decompose(self, bases: Collection[UnitBase] = ()) -> Self:
"""Generate a new instance with the physical unit decomposed.

For details, see `~astropy.units.Quantity.decompose`.
Expand Down
7 changes: 5 additions & 2 deletions astropy/units/quantity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from .utils import is_effectively_unity

if TYPE_CHECKING:
from collections.abc import Collection
from typing import Self

from .typing import QuantityLike
Expand Down Expand Up @@ -1593,7 +1594,7 @@ def __format__(self, format_spec):
# Format the whole thing as a single string.
return format(f"{self.value}{self._unitstr:s}", format_spec)

def decompose(self, bases=[]):
def decompose(self, bases: Collection[UnitBase] = ()) -> Self:
"""
Generates a new `Quantity` with the units
decomposed. Decomposed units have only irreducible units in
Expand All @@ -1615,7 +1616,9 @@ def decompose(self, bases=[]):
"""
return self._decompose(False, bases=bases)

def _decompose(self, allowscaledunits=False, bases=[]):
def _decompose(
self, allowscaledunits: bool = False, bases: Collection[UnitBase] = ()
) -> Self:
"""
Generates a new `Quantity` with the units decomposed. Decomposed
units have only irreducible units in them (see
Expand Down
9 changes: 8 additions & 1 deletion astropy/units/structured.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,23 @@
This module defines structured units and quantities.
"""

from __future__ import annotations

# Standard library
import operator
from functools import cached_property
from typing import TYPE_CHECKING

import numpy as np

from astropy.utils.compat.numpycompat import NUMPY_LT_1_24

from .core import UNITY, Unit, UnitBase

if TYPE_CHECKING:
from collections.abc import Collection
from typing import Self

__all__ = ["StructuredUnit"]


Expand Down Expand Up @@ -311,7 +318,7 @@ def physical_type(self):
operator.attrgetter("physical_type"), cls=Structure
)

def decompose(self, bases=set()):
def decompose(self, bases: Collection[UnitBase] = ()) -> Self:
"""The `StructuredUnit` composed of only irreducible units.

Parameters
Expand Down
3 changes: 3 additions & 0 deletions docs/nitpick-exceptions
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ py:class np.ma.MaskedArray
# locally defined type variable for ndarray dtype
py:class DT
# type aliases
py:class Complex
py:class Real
py:class UnitPower
Comment on lines +96 to +98
Copy link
Member Author

@eerovaher eerovaher Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTD was complaining about these. If we are using the aliases in public function signatures then we should document them, but I don't know how to do it. Just adding them to astropy.units.typing.__all__ wasn't good enough for Sphinx. These nitpick exceptions will have to do for now.

Copy link
Contributor

@mhvk mhvk Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd need to let sphinx typeset units.typing -- which would seem a good idea in general (if for another PR!). Should not need much more than adding it to docs/units/ref_api.rst (and ensuring the module docstring is clear about its purpose).

Copy link
Member Author

@eerovaher eerovaher Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I tried only editing astropy.units.typing.__all__ was that astropy.units.typing.QuantityLike is already documented, so Sphinx clearly already knows about astropy.units.typing.py.

Documenting type annotations with Sphinx is known to be buggy:

astropy/docs/nitpick-exceptions

Lines 80 to 82 in 4a55b10

# Types used in annotations included here because of a Sphinx bug.
# See https://github.com/sphinx-doc/sphinx/issues/9813
# and https://github.com/sphinx-doc/sphinx/issues/11225

It is quite likely that we cannot work around these bugs in astropy, so I don't intend on spending any more time looking into this.

Copy link
Contributor

@mhvk mhvk Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I see we expose units.typing already in type_hints.rst. So, makes sense just not worry about it for now.

py:class UnitScale

# Classes from `astropy.extern` that are nonetheless exposed in documentation
Expand Down
Loading