Fix mypy errors in xarray.py, xrutils.py, cache.py #144

Illviljan · 2022-09-19T20:05:59Z

Fixes some of #96

for more information, see https://pre-commit.ci

…im_typing

dcherian · 2022-09-19T20:10:41Z

flox/xarray.py

@@ -19,7 +19,10 @@
 from .xrutils import _contains_cftime_datetimes, _to_pytimedelta, datetime_to_numeric

 if TYPE_CHECKING:
-    from xarray import DataArray, Dataset, Resample
+    from xarray import DataArray, Dataset  # TODO: Use T_DataArray, T_Dataset?


Sure, we should explicitly say that xarray.types (is that right?) is public somewhere on the xarray docs.

It's xarray.core.types so I suppose it's technically private at the moment. Maybe for the better? I don't think .types has settled enough yet to start recommending to the larger audience. Doesn't stop us from using it early though! :)

I mainly wrote the ToDo because I had issues with mypy, but this was the solution:

# This errors if obj: T_Dataset | T_DataArray. if isinstance(obj, xr.DataArray): ds = obj._to_temp_dataset() else: ds = obj # This passes if obj: T_Dataset | T_DataArray. if isinstance(obj, xr.Dataset): ds = obj else: ds = obj._to_temp_dataset()

Great this would be a good issue to open over at xarray

The other reason this is fine is that I'd like to move the contents of this file over to xarray in the long term.

for more information, see https://pre-commit.ci

…im_typing

for more information, see https://pre-commit.ci

…im_typing

for more information, see https://pre-commit.ci

…im_typing

Illviljan · 2022-09-19T21:11:58Z

@headtr1ck, do you know why flake8 passes ellipsis on xarray but not here?

headtr1ck · 2022-09-19T21:21:52Z

@headtr1ck, do you know why flake8 passes ellipsis on xarray but not here?

It seems that flake8 does not support it yet fully and you have to convince it to expose it using this config in your setup.cfg:

[flake8]
builtins =
    ellipsis

flox/xarray.py

dcherian · 2022-09-19T20:32:50Z

flox/xarray.py

@@ -19,7 +19,10 @@
 from .xrutils import _contains_cftime_datetimes, _to_pytimedelta, datetime_to_numeric

 if TYPE_CHECKING:
-    from xarray import DataArray, Dataset, Resample
+    from xarray import DataArray, Dataset  # TODO: Use T_DataArray, T_Dataset?


Great this would be a good issue to open over at xarray

flox/xarray.py

for more information, see https://pre-commit.ci

dcherian

👍 thanks

flox/xarray.py

…im_typing

dcherian · 2022-09-20T21:43:14Z

flox/xarray.py

-    expected_groups = _convert_expected_groups_to_index(expected_groups, isbin, sort=sort)
-    group_shape = tuple(len(e) for e in expected_groups)
+    expected_groups = _convert_expected_groups_to_index(expected_groups, isbins, sort=sort)
+    # TODO: _convert_expected_groups_to_index can return None which is not good


expected_groups cannot have a None element at this stage see:

expected_groups[idx] = _get_expected_groups(b_.data, sort=sort, raise_if_dask=True)

This may be complicated from a typing perspective, so the comment should say that not describe a logic bug.

_get_expected_groups can also return None so it's not so easy to untangle this.
And even if expected_groups was narrowed properly it doesn't matter because _convert_expected_groups_to_index still has the None in it's return type. Example:

def test2(a: tuple[str | int, ...]) -> tuple[str | int, ...]: return a b: tuple[int, ...] = (1, 2) reveal_type(test2(a=b)) # note: Revealed type is "builtins.tuple[Union[builtins.str, builtins.int], ...]"

There may not be logic bug here but this part of the code is really hard to understand and could do with a little simplification.

👍 Agreed. One simplification would be to remove the raise_if_dask kwarg. It's only set to False in one place, we can explicitly skip it there.

Typing _convert_expected_groups_to_index is hard because it handles some very flexible user input but happy to hear suggestions.

I moved similar stuff inside the loop which simplified it a little e73f6e8
group_names can probably be replaced by group_sizes as well.

Co-authored-by: Deepak Cherian <[email protected]>

for more information, see https://pre-commit.ci

[pull] main from xarray-contrib:main

Illviljan · 2022-09-21T21:48:19Z

I think I'll stop here. core.py can be done in a different PR.

Here's what left in core.py:

flox/core.py:557: error: Incompatible default for argument "axis" (default has type "None", argument has type "Union[int, Sequence[int]]")  [assignment]
flox/core.py:612: error: Incompatible types in assignment (expression has type "Tuple[None]", variable has type "Optional[Mapping[Union[str, Callable[..., Any]], Any]]")  [assignment]
flox/core.py:668: error: No overload variant of "zip" matches argument types "Union[Sequence[str], Sequence[Callable[..., Any]]]", "None", "Any", "Any"  [call-overload]
flox/core.py:668: note: Possible overload variants:
flox/core.py:668: note:     def [_T_co, _T1] __new__(cls, Iterable[_T1], *, strict: bool = ...) -> zip[Tuple[_T1]]
flox/core.py:668: note:     def [_T_co, _T1, _T2] __new__(cls, Iterable[_T1], Iterable[_T2], *, strict: bool = ...) -> zip[Tuple[_T1, _T2]]
flox/core.py:668: note:     def [_T_co, _T1, _T2, _T3] __new__(cls, Iterable[_T1], Iterable[_T2], Iterable[_T3], *, strict: bool = ...) -> zip[Tuple[_T1, _T2, _T3]]
flox/core.py:668: note:     def [_T_co, _T1, _T2, _T3, _T4] __new__(cls, Iterable[_T1], Iterable[_T2], Iterable[_T3], Iterable[_T4], *, strict: bool = ...) -> zip[Tuple[_T1, _T2, _T3, _T4]]
flox/core.py:668: note:     def [_T_co, _T1, _T2, _T3, _T4, _T5] __new__(cls, Iterable[_T1], Iterable[_T2], Iterable[_T3], Iterable[_T4], Iterable[_T5], *, strict: bool = ...) -> zip[Tuple[_T1, _T2, _T3, _T4, _T5]]
flox/core.py:668: note:     def [_T_co] __new__(cls, Iterable[Any], Iterable[Any], Iterable[Any], Iterable[Any], Iterable[Any], Iterable[Any], *iterables: Iterable[Any], strict: bool = ...) -> zip[Tuple[Any, ...]]
flox/core.py:672: error: Argument 1 to "is_nanlen" has incompatible type "None"; expected "Union[str, Callable[..., Any]]"  [arg-type]
flox/core.py:809: error: Unsupported left operand type for + ("Sequence[Any]")  [operator]
flox/core.py:811: error: Unsupported left operand type for + ("Sequence[Any]")  [operator]
flox/core.py:816: error: Incompatible return value type (got "Dict[str, Any]", expected "Dict[Union[str, Callable[..., Any]], Any]")  [return-value]
flox/core.py:816: note: Perhaps you need a type annotation for "results"? Suggestion: "Dict[Union[str, Callable[..., Any]], Any]"
flox/core.py:902: error: Incompatible types in assignment (expression has type "Dict[Union[str, Callable[..., Any]], Any]", variable has type "Dict[str, object]")  [assignment]
flox/core.py:918: error: "object" has no attribute "append"  [attr-defined]
flox/core.py:921: error: "object" has no attribute "append"  [attr-defined]
flox/core.py:928: error: Argument "fill_value" to "chunk_reduce" has incompatible type "Tuple[int]"; expected "Optional[Mapping[Union[str, Callable[..., Any]], Any]]"  [arg-type]
flox/core.py:944: error: "object" has no attribute "append"  [attr-defined]
flox/core.py:957: error: Argument "fill_value" to "chunk_reduce" has incompatible type "Tuple[Any]"; expected "Optional[Mapping[Union[str, Callable[..., Any]], Any]]"  [arg-type]
flox/core.py:962: error: "object" has no attribute "append"  [attr-defined]
flox/core.py:964: error: Incompatible return value type (got "Dict[str, object]", expected "Dict[Union[str, Callable[..., Any]], Any]")  [return-value]
flox/core.py:964: note: Perhaps you need a type annotation for "results"? Suggestion: "Dict[Union[str, Callable[..., Any]], Any]"
flox/core.py:1182: error: Argument 1 to "partial" has incompatible type "object"; expected "Callable[..., Any]"  [arg-type]
flox/core.py:1235: error: Item "None" of "Optional[Any]" has no attribute "to_numpy"  [union-attr]
flox/core.py:1252: error: Incompatible types in assignment (expression has type "Array", variable has type "Dict[Any, Any]")  [assignment]
flox/core.py:1312: error: Incompatible return value type (got "Tuple[Any, ...]", expected "Tuple[Optional[Any]]")  [return-value]
flox/core.py:1468: error: Argument 1 to "_validate_reindex" has incompatible type "Optional[bool]"; expected "bool"  [arg-type]
flox/core.py:1482: error: Incompatible types in assignment (expression has type "Tuple[bool, ...]", variable has type "bool")  [assignment]
flox/core.py:1498: error: Argument 2 to "_convert_expected_groups_to_index" has incompatible type "bool"; expected "Sequence[bool]"  [arg-type]
flox/core.py:1502: error: Argument 1 to "any" has incompatible type "bool"; expected "Iterable[object]"  [arg-type]
flox/core.py:1566: error: Argument 4 to "_initialize_aggregation" has incompatible type "Optional[int]"; expected "int"  [arg-type]
flox/core.py:1578: error: Item "str" of "Union[str, Aggregation]" has no attribute "name"  [union-attr]
flox/core.py:1590: error: Item "ndarray[Any, dtype[Any]]" of "Union[ndarray[Any, dtype[Any]], Any, ndarray[Any, Any]]" has no attribute "chunks"  [union-attr]
flox/core.py:1590: error: Item "ndarray[Any, Any]" of "Union[ndarray[Any, dtype[Any]], Any, ndarray[Any, Any]]" has no attribute "chunks"  [union-attr]
flox/core.py:1603: error: Item "ndarray[Any, dtype[Any]]" of "Union[ndarray[Any, dtype[Any]], Any, ndarray[Any, Any]]" has no attribute "chunks"  [union-attr]
flox/core.py:1603: error: Item "ndarray[Any, Any]" of "Union[ndarray[Any, dtype[Any]], Any, ndarray[Any, Any]]" has no attribute "chunks"  [union-attr]
flox/core.py:1634: error: Incompatible types in assignment (expression has type "List[Union[ndarray[Any, Any], Any]]", variable has type "Tuple[Any]")  [assignment]
Found 30 errors in 1 file (checked 10 source files)

dcherian

👏 👏 👏

nice work!

dcherian · 2022-09-22T03:43:03Z

flox/xarray.py

-    if isinstance(obj, xr.DataArray):
-        ds = obj._to_temp_dataset()
-    else:
+    if isinstance(obj, xr.Dataset):


this rearrangement was weird. Is it a mypy bug?

This is the error you get if you isinstance with DataArray:

# obj: Union[T_Dataset, T_DataArray] if isinstance(obj, xr.DataArray): ds = obj._to_temp_dataset() # -> xr.Dataset else: ds = obj # error: Incompatible types in assignment (expression has type "Union[T_Dataset, T_DataArray]", variable has type "Dataset")

My understanding is that mypy always uses the typing from the first time it is defined (ds: xr.Dataset narrower typing). It is similar to the typing issues when importing optional modules

flox/xarray.py

Co-authored-by: Deepak Cherian <[email protected]>

* main: Update ci-additional.yaml (#167) Refactor before redoing cohorts (#164) Fix mypy errors in core.py (#150) Add link to numpy_groupies (#160) Bump codecov/codecov-action from 3.1.0 to 3.1.1 (#159) Use math.prod instead of np.prod (#157) Remove None output from _get_expected_groups (#152) Fix mypy errors in xarray.py, xrutils.py, cache.py (#144) Raise error if multiple by's are used with Ellipsis (#149) pre-commit autoupdate (#148) Add mypy ignores (#146) Get pre commit bot to update (#145) Remove duplicate examples headers (#147) Add ci additional (#143) Bump mamba-org/provision-with-micromamba from 12 to 13 (#141) Add ASV benchmark CI workflow (#139) Fix func count for dtype O with numpy and numba (#138)

* main: (29 commits) Major fix to subset_to_blocks (#173) Performance improvements for cohorts detection (#172) Remove split_out (#170) Deprecate resample_reduce (#169) More efficient cohorts. (#165) Allow specifying output dtype (#131) Add a dtype check for numpy arrays in assert_equal (#158) Update ci-additional.yaml (#167) Refactor before redoing cohorts (#164) Fix mypy errors in core.py (#150) Add link to numpy_groupies (#160) Bump codecov/codecov-action from 3.1.0 to 3.1.1 (#159) Use math.prod instead of np.prod (#157) Remove None output from _get_expected_groups (#152) Fix mypy errors in xarray.py, xrutils.py, cache.py (#144) Raise error if multiple by's are used with Ellipsis (#149) pre-commit autoupdate (#148) Add mypy ignores (#146) Get pre commit bot to update (#145) Remove duplicate examples headers (#147) ...

Illviljan and others added 6 commits September 17, 2022 06:04

update dim typing

c972e97

Merge branch 'main' into dim_typing

2e42456

Fix mypy errors in xarray.py

64c7d77

[pre-commit.ci] auto fixes from pre-commit.com hooks

b3d698a

for more information, see https://pre-commit.ci

start mypy ci

6e4db03

Merge branch 'dim_typing' of https://github.com/Illviljan/flox into d…

afee7c4

…im_typing

Illviljan changed the title ~~Dim typing~~ Fix mypy errors in xarray.py Sep 19, 2022

dcherian reviewed Sep 19, 2022

View reviewed changes

Illviljan and others added 10 commits September 19, 2022 22:31

Use T_DataArray and T_Dataset

ed752dd

[pre-commit.ci] auto fixes from pre-commit.com hooks

6303f4a

for more information, see https://pre-commit.ci

Add mypy ignores

ae8953a

Merge branch 'dim_typing' of https://github.com/Illviljan/flox into d…

8fba166

…im_typing

[pre-commit.ci] auto fixes from pre-commit.com hooks

ae5561d

for more information, see https://pre-commit.ci

correct typing a bit

5145dc2

Merge branch 'dim_typing' of https://github.com/Illviljan/flox into d…

5d46140

…im_typing

[pre-commit.ci] auto fixes from pre-commit.com hooks

05893a2

for more information, see https://pre-commit.ci

test newer flake8 if ellipsis passes there

375c31b

Merge branch 'dim_typing' of https://github.com/Illviljan/flox into d…

6ba6da4

…im_typing

Allow ellipsis in flake8

170467b

Illviljan commented Sep 19, 2022

View reviewed changes

flox/xarray.py Outdated Show resolved Hide resolved

Update core.py

a3d63a2

dcherian reviewed Sep 19, 2022

View reviewed changes

Illviljan added 6 commits September 20, 2022 18:19

Update xarray.py

cf0d6cd

Merge branch 'main' into dim_typing

bde6c52

Update setup.cfg

3728858

Update xarray.py

657496d

Update xarray.py

68ac242

Update xarray.py

c306099

Illviljan and others added 2 commits September 20, 2022 23:12

hopefully no more pytest errors.

1accd73

[pre-commit.ci] auto fixes from pre-commit.com hooks

a50bb6b

for more information, see https://pre-commit.ci

dcherian reviewed Sep 20, 2022

View reviewed changes

flox/xarray.py Outdated Show resolved Hide resolved

flox/xarray.py Show resolved Hide resolved

flox/xarray.py Outdated Show resolved Hide resolved

flox/xarray.py Outdated Show resolved Hide resolved

Illviljan added 2 commits September 20, 2022 23:39

make sure expected_groups doesn't have None

50c2ac2

Merge branch 'dim_typing' of https://github.com/Illviljan/flox into d…

db2ac1b

…im_typing

dcherian reviewed Sep 20, 2022

View reviewed changes

Illviljan and others added 7 commits September 21, 2022 00:04

Update flox/xarray.py

1921938

Co-authored-by: Deepak Cherian <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

3cac4b0

for more information, see https://pre-commit.ci

ds_broad and longer comment

43dabff

Use same for loop for similar things.

e73f6e8

[pre-commit.ci] auto fixes from pre-commit.com hooks

2d62748

for more information, see https://pre-commit.ci

Merge pull request #31 from xarray-contrib/main

62cc554

[pull] main from xarray-contrib:main

fix xrutils.py

41e97e9

Illviljan changed the title ~~Fix mypy errors in xarray.py~~ Fix mypy errors in xarray.py, xrutils.py Sep 21, 2022

Illviljan added 2 commits September 21, 2022 23:34

fix errors in cache.py

fc36211

Merge branch 'main' into dim_typing

a5d41a5

Illviljan changed the title ~~Fix mypy errors in xarray.py, xrutils.py~~ Fix mypy errors in xarray.py, xrutils.py, cache.py Sep 21, 2022

Turn off mypy check

bfb9c6e

Illviljan marked this pull request as ready for review September 21, 2022 21:49

Illviljan mentioned this pull request Sep 21, 2022

Raise error if multiple by's are used with Ellipsis #149

Merged

dcherian approved these changes Sep 22, 2022

View reviewed changes

Illviljan and others added 5 commits September 22, 2022 18:06

Update flox/xarray.py

7260660

Co-authored-by: Deepak Cherian <[email protected]>

Update flox/xarray.py

b34c268

Co-authored-by: Deepak Cherian <[email protected]>

Use if else format to avoid tuple creation

eaf93d2

Update xarray.py

9486184

Merge branch 'main' into dim_typing

b18d209

Illviljan merged commit 2b54c5e into xarray-contrib:main Sep 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mypy errors in xarray.py, xrutils.py, cache.py #144

Fix mypy errors in xarray.py, xrutils.py, cache.py #144

Illviljan commented Sep 19, 2022 •

edited

Loading

dcherian Sep 19, 2022

Illviljan Sep 19, 2022

dcherian Sep 19, 2022

dcherian Sep 20, 2022

Illviljan commented Sep 19, 2022

headtr1ck commented Sep 19, 2022

dcherian Sep 19, 2022

dcherian left a comment

dcherian Sep 20, 2022 •

edited

Loading

Illviljan Sep 20, 2022 •

edited

Loading

dcherian Sep 21, 2022

Illviljan Sep 21, 2022

Illviljan commented Sep 21, 2022

dcherian left a comment

dcherian Sep 22, 2022

Illviljan Sep 22, 2022

Fix mypy errors in xarray.py, xrutils.py, cache.py #144

Fix mypy errors in xarray.py, xrutils.py, cache.py #144

Conversation

Illviljan commented Sep 19, 2022 • edited Loading

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

Illviljan commented Sep 19, 2022

headtr1ck commented Sep 19, 2022

Choose a reason for hiding this comment

dcherian left a comment

Choose a reason for hiding this comment

dcherian Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Illviljan Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Illviljan commented Sep 21, 2022

dcherian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Illviljan commented Sep 19, 2022 •

edited

Loading

dcherian Sep 20, 2022 •

edited

Loading

Illviljan Sep 20, 2022 •

edited

Loading