[Feat] Add type hints #251

fjosw · 2024-12-25T10:28:13Z

As a first step towards adding type hints, I added automatic type hints with monkeytype. Hints for the more complicated methods will need additional manual work but for the simpler methods this seems to have worked quite well.

compatability

fjosw · 2025-01-03T18:16:25Z

Not quite done but already in decent shape for a review @s-kuberski @jkuhl-uni:

Most of the changes are just type annotations
I changed a few lines to help mypy with understanding the type annotations
I fixed a few smaller bugs in which explicit type checks were not consistent or did not cover certain edge cases

For now I ignored the input/* modules and most of correlators.py, the remaining modules only raise a few smaller mypy errors which I wasn't able to fix right away.

s-kuberski · 2025-01-05T14:32:54Z

Great, thanks a lot for all of your work! I'll have a look in the upcoming week.

jkuhl-uni · 2025-01-09T09:32:26Z

pyerrors/correlators.py

@@ -42,7 +45,7 @@ class Corr:

    __slots__ = ["content", "N", "T", "tag", "prange"]

-    def __init__(self, data_input, padding=[0, 0], prange=None):
+    def __init__(self, data_input: Any, padding: list[int]=[0, 0], prange: Optional[list[int]]=None):


Shouldn't this be list[Obs]? Alternatively, in the list, there could be something Obs-like, I suppose, such as list[CObs]?

jkuhl-uni · 2025-01-09T09:54:35Z

Hey @fjosw,
thanks for all the work, I am looking through it right now, it looks excellent to me. I hope it was okay that I pushed a few small changes. If not, feel free to revert them, as you please :)

s-kuberski · 2025-01-10T14:04:33Z

pyerrors/correlators.py

        """Calculates the per-timeslice trace of a correlator matrix."""
        if self.N == 1:
            raise ValueError("Only works for correlator matrices.")
-        newcontent = []
+        newcontent: list[Union[None, float]] = []


Similar to Justus' question above: newcontent should be a list of NoneorObs(orCObs`?), right?

s-kuberski · 2025-01-10T14:11:22Z

pyerrors/correlators.py

@@ -701,7 +698,7 @@ def second_deriv(self, variant="symmetric"):
        else:
            raise ValueError("Unknown variant.")

-    def m_eff(self, variant='log', guess=1.0):
+    def m_eff(self, variant: str='log', guess: float=1.0) -> "Corr":


A very naive question and probably that is just something that was automatically done: When shoult it be Optional[str] (as above) and when str (as here)?

I think it should be Optional[str] when the value can also be None, I think in this case one really needs to pass a string, right?

Optional[X] is equivalent to X | None (or Union[X, None]).

https://docs.python.org/3/library/typing.html#typing.Optional

s-kuberski · 2025-01-10T14:12:48Z

pyerrors/correlators.py

@@ -785,7 +782,7 @@ def root_function(x, d):
        else:
            raise ValueError('Unknown variant.')

-    def fit(self, function, fitrange=None, silent=False, **kwargs):
+    def fit(self, function: Callable, fitrange: Optional[Union[str, list[int]]]=None, silent: bool=False, **kwargs) -> Fit_result:


I don't see that fitrange can be a string.

s-kuberski · 2025-01-10T14:13:46Z

pyerrors/correlators.py

@@ -819,7 +816,7 @@ def fit(self, function, fitrange=None, silent=False, **kwargs):
        result = least_squares(xs, ys, function, silent=silent, **kwargs)
        return result

-    def plateau(self, plateau_range=None, method="fit", auto_gamma=False):
+    def plateau(self, plateau_range: Optional[list[int]]=None, method: str="fit", auto_gamma: bool=False) -> Obs:


As above, this is an optional str, but I guess this is not really important.

s-kuberski · 2025-01-10T14:16:31Z

pyerrors/correlators.py

@@ -868,7 +865,7 @@ def set_prange(self, prange):
        self.prange = prange
        return

-    def show(self, x_range=None, comp=None, y_range=None, logscale=False, plateau=None, fit_res=None, fit_key=None, ylabel=None, save=None, auto_gamma=False, hide_sigma=None, references=None, title=None):
+    def show(self, x_range: Optional[list[int]]=None, comp: Optional[Corr]=None, y_range: None=None, logscale: bool=False, plateau: None=None, fit_res: Optional[Fit_result]=None, fit_key: Optional[str]=None, ylabel: None=None, save: None=None, auto_gamma: bool=False, hide_sigma: None=None, references: None=None, title: None=None):


The types of y_range, plateau, ylabel, save, hide_sigma, references, title have not been recognized.

When a type has not been recognised, this indicates that we don't have proper test coverage for this call 😬

s-kuberski · 2025-01-10T14:19:30Z

pyerrors/correlators.py

@@ -1084,18 +1081,18 @@ def __str__(self):

    __array_priority__ = 10000

-    def __eq__(self, y):
+    def __eq__(self, y: Union[Corr, Obs, int]) -> ndarray:


I guess y can be Any?

s-kuberski · 2025-01-10T14:20:57Z

pyerrors/correlators.py

        if isinstance(y, Corr):
            comp = np.asarray(y.content, dtype=object)
        else:
            comp = np.asarray(y)
        return np.asarray(self.content, dtype=object) == comp

-    def __add__(self, y):
+    def __add__(self, y: Any) -> "Corr":


In contrast, here, y should be one of the types that don't raise an exception.

s-kuberski · 2025-01-10T14:21:26Z

pyerrors/correlators.py

@@ -1119,11 +1116,11 @@ def __add__(self, y):
        else:
            raise TypeError("Corr + wrong type")

-    def __mul__(self, y):
+    def __mul__(self, y: Any) -> "Corr":


s-kuberski · 2025-01-10T14:23:07Z

pyerrors/correlators.py

@@ -1190,11 +1187,11 @@ def __rmatmul__(self, y):
        else:
            return NotImplemented

-    def __truediv__(self, y):
+    def __truediv__(self, y: Union[Corr, float, ndarray, int]) -> "Corr":


Obs and CObs are missing here. BTW: does that just mean that we don't call the function with these types in our tests?

Yes, that's my understanding. monkeytype derived these type hints from our tests.

s-kuberski · 2025-01-10T14:24:46Z

pyerrors/correlators.py

        newcontent = [None if _check_for_none(self, item) else -1. * item for item in self.content]
        return Corr(newcontent, prange=self.prange)

-    def __sub__(self, y):
+    def __sub__(self, y: Union[Corr, float, ndarray, int]) -> "Corr":


incomplete list for y.

Okay, I stop here for the rest of the file. There are a few more instances below, where we have to adjust `y.

s-kuberski · 2025-01-10T14:27:36Z

pyerrors/correlators.py

@@ -1356,7 +1353,7 @@ def return_imag(obs_OR_cobs):

        return self._apply_func_to_corr(return_imag)

-    def prune(self, Ntrunc, tproj=3, t0proj=2, basematrix=None):
+    def prune(self, Ntrunc: int, tproj: int=3, t0proj: int=2, basematrix: None=None) -> "Corr":


basematrix should be an Optional[ndarray].

s-kuberski · 2025-01-10T14:31:08Z

Thansk again for starting this. I'm still in correlators.py and have discovered quite a few places, where I think that the type hints should be changed, if one wants them to be correct. What's our strategy? Shall I continue to point them out and you change them if you agree? Or shall I directly adapt? I guess it's not breaking anything if some of the hints are incorrect, but if we introduce them in the first place, I think we should make an effort to have them correct and hopefully complete.

fjosw · 2025-01-10T14:35:21Z

Feel free to get started with fixing stuff, you should have permissions to push to the branch, I think I won't have time to work on this in the comming days. My strategy so far was to run mypy to point out typing issues and go from there. As @jkuhl-uni suggested it might be worth defining a Obs-like type which can then be used for the overloaded methods.

fjosw added 12 commits December 25, 2024 11:09

[Feat] Add type hints to pyerrors modules

3db8eb2

[Feat] Added type hints to input modules

9fe375a

[Feat] Fixed a few type hints manually

8d86295

[Fix] Fix ruff errors and a few type annotations

2d34b35

[Fix] Fix type annotations for first part of obs.py

a9e082c

[Fix] Fixed most type annotations in obs.py

0198256

[CI] Add typing extension package to pytest run for pytthon 3.9

9389ad6

[Fix] Fix type hints in misc.py and remove strict zips for python 3.9

23d4f4c

compatability

[Fix] Start fixing remaining type hints

1916de1

[Fix] Removed unused imports

3654635

[CI] Add E252 to flake8 exceptions

4f1606d

[Fix] Fixed remaining flake8 errors

d45b43e

fjosw marked this pull request as ready for review January 3, 2025 18:12

fjosw requested review from jkuhl-uni and s-kuberski as code owners January 3, 2025 18:12

fjosw added 4 commits January 3, 2025 22:43

[Fix] Simplify type hints

1c6053e

[Fix] Fix type annotations in json.py

b8700ef

[Fix] Simplify type annotations in input modules

6d5a9b9

[Fix] Fix ruff

6a990c1

fjosw mentioned this pull request Jan 3, 2025

ens_name not correctly set in sfcf module #252

Closed

fjosw and others added 5 commits January 5, 2025 16:30

[Fix] Correct type hints in fits.py

9c960ae

[Fix] Further type fixes in fits and sfcf

5376a8a

[Fix] More type hint fixing

336117c

Merge branch 'develop' into feat/typehints

dba277f

add typehints for other util functions

52b91d8

jkuhl-uni reviewed Jan 9, 2025

View reviewed changes

make deriv structure like second_deriv

cb9d942

add typing for read_pbp

2f40ff8

s-kuberski reviewed Jan 10, 2025

View reviewed changes

[Fix] Additional typehints

bbf0b68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat] Add type hints #251

[Feat] Add type hints #251

fjosw commented Dec 25, 2024

fjosw commented Jan 3, 2025 •

edited

Loading

s-kuberski commented Jan 5, 2025

jkuhl-uni Jan 9, 2025 •

edited

Loading

jkuhl-uni commented Jan 9, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

fjosw Jan 10, 2025

fjosw Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

fjosw Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

fjosw Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski Jan 10, 2025

s-kuberski commented Jan 10, 2025

fjosw commented Jan 10, 2025

[Feat] Add type hints #251

Are you sure you want to change the base?

[Feat] Add type hints #251

Conversation

fjosw commented Dec 25, 2024

fjosw commented Jan 3, 2025 • edited Loading

s-kuberski commented Jan 5, 2025

jkuhl-uni Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

jkuhl-uni commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kuberski commented Jan 10, 2025

fjosw commented Jan 10, 2025

fjosw commented Jan 3, 2025 •

edited

Loading

jkuhl-uni Jan 9, 2025 •

edited

Loading