-
Notifications
You must be signed in to change notification settings - Fork 16
[Feat] Add type hints #251
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
base: develop
Are you sure you want to change the base?
Conversation
Not quite done but already in decent shape for a review @s-kuberski @jkuhl-uni:
For now I ignored the |
Great, thanks a lot for all of your work! I'll have a look in the upcoming week. |
pyerrors/correlators.py
Outdated
@@ -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): |
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.
Shouldn't this be list[Obs]
? Alternatively, in the list, there could be something Obs-like, I suppose, such as list[CObs]
?
Hey @fjosw, |
pyerrors/correlators.py
Outdated
@@ -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: |
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 don't see that fitrange
can be a string.
@@ -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: |
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.
As above, this is an optional str
, but I guess this is not really important.
pyerrors/correlators.py
Outdated
@@ -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): |
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.
The types of y_range
, plateau
, ylabel
, save
, hide_sigma
, references
, title
have not been recognized.
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.
When a type has not been recognised, this indicates that we don't have proper test coverage for this call 😬
pyerrors/correlators.py
Outdated
@@ -1084,18 +1081,18 @@ def __str__(self): | |||
|
|||
__array_priority__ = 10000 | |||
|
|||
def __eq__(self, y): | |||
def __eq__(self, y: Union[Corr, Obs, int]) -> ndarray: |
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 guess y
can be Any
?
pyerrors/correlators.py
Outdated
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": |
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.
In contrast, here, y
should be one of the types that don't raise an exception.
pyerrors/correlators.py
Outdated
@@ -1119,11 +1116,11 @@ def __add__(self, y): | |||
else: | |||
raise TypeError("Corr + wrong type") | |||
|
|||
def __mul__(self, y): | |||
def __mul__(self, y: Any) -> "Corr": |
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.
as above
pyerrors/correlators.py
Outdated
@@ -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": |
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.
Obs
and CObs
are missing here. BTW: does that just mean that we don't call the function with these types in our tests?
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.
Yes, that's my understanding. monkeytype derived these type hints from our tests.
pyerrors/correlators.py
Outdated
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": |
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.
incomplete list for y
.
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.
Okay, I stop here for the rest of the file. There are a few more instances below, where we have to adjust `y.
pyerrors/correlators.py
Outdated
@@ -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": |
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.
basematrix
should be an Optional[ndarray]
.
Thansk again for starting this. I'm still in |
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 |
I've tried to go through the changes and to address the issues that I raise above and a few more things. |
I resolved the merge conflicts and fixed a few smaller issues but now one of the tests |
I had a look and found a solution for now, there are a few minor things to figure out. I hope to get it done today :) |
... well that was easy. |
Thanks a lot @jkuhl-uni ! As we have accumulated quite a few changes I would suggest that we all do a thorough review and then merge this first version with type hints which we can refine later @s-kuberski @jkuhl-uni . |
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.