chore: cleanup of get_dtype (with numpy 2.5 support) - #285
chore: cleanup of get_dtype (with numpy 2.5 support)#285vincentkoppen wants to merge 13 commits into
Conversation
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
Signed-off-by: jaapschoutenalliander <jaap.schouten@alliander.com>
Signed-off-by: Vincent Koppen <53343926+vincentkoppen@users.noreply.github.com>
Signed-off-by: jaapschoutenalliander <jaap.schouten@alliander.com>
Signed-off-by: jaapschoutenalliander <jaap.schouten@alliander.com>
Signed-off-by: jaapschoutenalliander <jaap.schouten@alliander.com>
Signed-off-by: jaapschoutenalliander <jaap.schouten@alliander.com>
|
vincentkoppen
left a comment
vincentkoppen
left a comment
There was a problem hiding this comment.
Looks good to me overall.
Just some nitpicks, nothing major. Take a look if you agree with any of them, otherwise this is good to.
(can't approve, since it's my PR)
| # Expected type_args for NDArray3: (NDArray[numpy.float64], typing.Literal[3]) | ||
| if len(type_args) == 2: # noqa: PLR2004 | ||
| dtype = get_args(type_args[0])[0] | ||
| return (name, _resolve_str_dtype(name, dtype, str_lengths), get_args(type_args[1])[0]) |
There was a problem hiding this comment.
nitpick: personally I would prefer to apply _resolve_str_dtype after the parsing.
So: parse all dtypes -> apply the str filter over all -> return
The str parsing isn't specific to the parsing per numpy version. Now this has to be applied 4 times and is possiby easily forgotten if a new parsing is added.
| raise ValueError(f"dtype {type_def} not understood or supported") | ||
|
|
||
|
|
||
| def _parse_annotation_post_25(name: str, type_def: Any, type_args: tuple, str_lengths: dict[str, int]) -> tuple: |
There was a problem hiding this comment.
nitpick: sonarcloud isn't happy with the return type. I think we could do something like tuple[str, np.generic | str,...] or more explicit: tuple[str, np.generic | str] | tuple[str, np.generic | str, int]
The | str is only needed if we keep the string conversion. Also it might need type[np.generic] instead of np.generic, not sure.
| return dtype | ||
|
|
||
|
|
||
| def _parse_annotation_pre_25(name: str, type_def: Any, type_args: tuple, str_lengths: dict[str, int]) -> tuple: |
There was a problem hiding this comment.
Same for this fopr the return type.
| Self = TypeVar("Self", bound="FancyArray") | ||
|
|
||
|
|
||
| def _resolve_str_dtype(name: str, dtype: Any, str_lengths: dict[str, int]) -> Any: |
There was a problem hiding this comment.
Typing can be improved here as well I think, if np.generic or type[np.generic] is correct in the previous comments, then it can also be used here.
| dtype_list = [ | ||
| parse_annotation(name, type_def, get_args(type_def), str_lengths) for name, type_def in annotations.items() | ||
| ] | ||
|
|
There was a problem hiding this comment.
- nitpick: Prefer to use keyword args
- The fact that we have both type_def and type_args feels a strange. But I think this is needed mainly for the tests right?
Otherwise it's hard to test both python versions with only passingtype_def?
Fine to keep it as is, but I would also be fine with:
dtype_list = []
for name, dtype in annotations.items():
try:
dtype_args = get_args(dtype)
dtype_list.append(parse_annotations(name, type_args, ...))
exception:
raise ValueError(".... {type_def}....")
2 participants
A small cleanup of the get_dtype function:
annotationsinstead of thedtypesdictdtypesdict, directly create thedtype_listShould make Sonarcloud happy and make the flow a bit simpler to understand.