Fix type hints #255
Fix type hints #255
Conversation
|
$ mypy modelskill/comparison/
Success: no issues found in 6 source files |
$ mypy modelskill/
Success: no issues found in 29 source filesWhich translates to no unknown issues, there are still plenty of lines with a This PR allows us to use type hints with more confidence, since future changes can be verified with |
| @@ -20,7 +20,7 @@ def scatter( | |||
| self, | |||
| *, | |||
| model=None, | |||
| bins: Union[int, float, List[int], List[float]] = 20, | |||
There was a problem hiding this comment.
Why are these removed - matplotlib allows to give edges as a list - (actually str also works) https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.hist.html
There was a problem hiding this comment.
There were no tests with bins as a list of values. And when I tried it failed to produce a scatter plot
The type hints should reflect working functionality (== tested).
There was a problem hiding this comment.
@daniel-caichac-DHI can you help here - are the bins as a list of edges needed?
There was a problem hiding this comment.
I like to have automatic bins but sometimes someone might want to reproduce an old plot or a plot with given bins (eg, bins that are not equidistant, logarithmic bins (for whatever reason?)
I'll try to see if if works with bins= list of values
There was a problem hiding this comment.
I guess also when you want consistent plots with the same bins across many plots?
| def scatter( | ||
| self, | ||
| *, | ||
| model=None, | ||
| bins: Union[int, float, List[int], List[float]] = 20, |
There was a problem hiding this comment.
see above - and actually "float" is not allowed?? https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.hist.html
There was a problem hiding this comment.
Let's fix this in a separate PR
| by: Union[str, List[str]] = None, | ||
| metrics: list = None, | ||
| by: Optional[Union[str, List[str]]] = None, | ||
| metrics: Optional[list] = None, |
| # TODO does this work as intended? Mutable default values, e.g. dict, list, are usually not recommended | ||
| register_option( | ||
| key="plot.rcParams", defval={}, validator=settings.is_dict | ||
| ) # still have to |
|
|
||
| if not isinstance(quantiles, (int, Sequence)): |
There was a problem hiding this comment.
You removed this - but what if quantiles is float or str?
which in some cases, means removing the type hints or muting it with
# type : ignore.