-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
gh-358: add static types support #368
base: main
Are you sure you want to change the base?
Conversation
@@ -4,3 +4,4 @@ b42067f6776dcd763827000d585a88e071b78af3 | |||
f9bac62f497a7288aa71fd4dbd948945c37f854e | |||
# applied ruff-linting - https://github.com/glass-dev/glass/pull/232 | |||
e58d8616c03b8447aba90996905a98b42f18ba0a | |||
# added static typing - https://github.com/glass-dev/glass/pull/368 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reminder, I should add the hash once merged
@@ -104,6 +105,7 @@ plugins = [ | |||
"numpy.typing.mypy_plugin", | |||
] | |||
strict = true | |||
warn_return_any = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 17 errors which are ignored by this. They are almost all related to untyped external libraries. Do we want this here or the individual # type: ignore[no-any-return]
comments?
w = RadialWindow(za=[1.0, 2.0, 3.0, 4.0], wa=[0.0, 0.5, 0.5, 0.0], zeff=None) # type: ignore[arg-type] | ||
w = RadialWindow( | ||
za=np.array([1.0, 2.0, 3.0, 4.0]), | ||
wa=np.array([0.0, 0.5, 0.5, 0.0]), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the default zeff
to be 0
as None
didn't make sense as a type
def _test_append( | ||
fits: fitsio.FITS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this here as I wanted fitsio
for typing
if importlib.util.find_spec("fitsio") is not None: | ||
import fitsio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit hacky, but seems to work for mypy
if self.ext is None or self.ext not in self.fits: | ||
if self.ext not in self.fits: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now self.ext
defaults to ""
then no harm in having a blank extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But those are different situations: a blank EXTNAME is not the same as not having an EXTNAME.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the function (as I understand it) will add "", which if it already has one isn't going to do anything. I can change this to
if not self.ext or self.ext not in self.fits:
it just felt redundant
Size = typing.Optional[typing.Union[int, tuple[int, ...]]] | ||
Iternorm = tuple[typing.Optional[int], npt.NDArray[typing.Any], npt.NDArray[typing.Any]] | ||
ClTransform = typing.Union[ | ||
str, typing.Callable[[npt.NDArray[typing.Any]], npt.NDArray[typing.Any]] | ||
] | ||
Cls = collections.abc.Sequence[ | ||
typing.Union[npt.NDArray[typing.Any], collections.abc.Sequence[float]] | ||
] | ||
Alms = npt.NDArray[typing.Any] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of these as I don't really like custom types. Can return if there are strong feelings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I suspect mypy
won't pass if all have the same type, i.e. Cls
do vary depending on context
return sum( | ||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sum(<x>)
is equivalent to np.sum(<x>, axis=0)
and is more accurately showing what is happening + fixes mypy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First and foremost: monumental work @paddyroddy!
I only managed to make it down to observations.py
before I have to jump on the next round of calls, but leaving a first set comments here in the meantime. Main issue I've seen is that a) some functions changed behaviour, and b) the type of cls
/gls
arguments is incorrect: these are ragged sequences of arrays, for example [np.array(...), np.array(...), []]
for the cls
of two shells with no cross-correlation.
Anyway, excellent work and titanic effort! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also make sense to me to include only glass/
in the type checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the notebooks being formatted in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is really a change that needs to happen in this PR.
def cumtrapz( | ||
f: npt.NDArray[np.int_] | npt.NDArray[np.float64], | ||
x: npt.NDArray[np.int_] | npt.NDArray[np.float64], | ||
dtype: type | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to use DTypeLike
size: Size = None, | ||
) -> collections.abc.Generator[Iternorm, None, None]: | ||
cov: collections.abc.Iterable[npt.NDArray[np.float64]], | ||
size: tuple[int, ...] = (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was correct before: size
has type int | tuple[int, ...] | None
and default should remain None
, which is semantically different from ()
pairs = ( | ||
combinations_with_replacement(np.ndindex(shape1[1:]), 2) | ||
if weights2 is weights1 | ||
else product(np.ndindex(shape1[1:]), np.ndindex(shape2[1:])) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this way harder to read, but I understand you don't want to manually type pairs
here
dims, count, z, nz = broadcast_leading_axes((count, 0), (z, 1), (nz, 1)) # type: ignore[no-untyped-call] | ||
dims, *rest = broadcast_leading_axes((count, 0), (z, 1), (nz, 1)) | ||
count_out, z_out, nz_out = rest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should not be necessary if broadcast_leading_axes()
is typed correctly
shells: collections.abc.Sequence[RadialWindow], | ||
shells: list[RadialWindow], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works with any sequence though
weights: npt.ArrayLike, | ||
shells: collections.abc.Sequence[RadialWindow], | ||
weights: npt.NDArray[np.float64], | ||
shells: list[RadialWindow], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above, no need to restrict it to a list (and it's not always a list in practice)
mean: npt.NDArray[np.float64], | ||
sigma: npt.NDArray[np.float64], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should accept float
since they are in practice often called with single float arguments
Closes #358. Replaces #308.
This PR adds static type support, hence satisfying
mypy
. With the adaptation of #67 I anticipate typing to change, but it's much easier to do oncemypy
is passing initially.The only
# type: ignore[]
comments remaining areattr-defined
which are all attributed tonp.trapz
, those can easily be fixed by #207.I have turned off:
disallow_untyped_decorators
this is being triggered bypytest
andnox
. The equivalent# type: ignore[]
error ismisc
which isn't the most informativewarn_return_any
which is due to external libraries likecosmology
- this could be revisited in the futureI have decided to re-write some of the tests which were simply not following the typing - like passing is a
list
rather than anp.ndarray
. The alternative approach would be having really messy typing.I'm not expecting this to be fully perfect. We will likely have to iterate on this, and completely change once we follow #67. But this is a good first step.
Do make sure to look carefully. Hard to avoid such a big PR with this. There were over 200
# type: ignore[]
comments to fix!