Skip to content
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

Typing for open_dataset/array/mfdataset and to_netcdf/zarr #6612

Merged
merged 54 commits into from
May 17, 2022

Conversation

headtr1ck
Copy link
Collaborator

Mypy is not able to compute the overloads of to_netcdf properly (too many Unions).
I had to add some # type: ignore

I am not sure about the types of

  • concat_dim
  • combine_attrs: the Callable part is still with Anys

@headtr1ck headtr1ck changed the title Typing for open_dataset/array/mfdataset and to_netcdf Typing for open_dataset/array/mfdataset and to_netcdf/zarr May 15, 2022
Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Herculean effort on this!

Could we add some -> None to some of the methods in test_backends.py so we can test the annotations? Particularly for these methods "at the edge", they're unlikely to be called by many methods within xarray, and so really the only way to test them is to add annotations to tests.

Thanks a lot @headtr1ck !

xarray/backends/api.py Outdated Show resolved Hide resolved
xarray/backends/api.py Show resolved Hide resolved
xarray/backends/api.py Outdated Show resolved Hide resolved
xarray/backends/api.py Outdated Show resolved Hide resolved
xarray/backends/api.py Outdated Show resolved Hide resolved
xarray/backends/api.py Show resolved Hide resolved
@max-sixty
Copy link
Collaborator

I think merging main may fix the benchmark errors. (If you enable permissions for maintainers to modify the PR then GH offers a button to do this, FWIW)

@headtr1ck
Copy link
Collaborator Author

I think merging main may fix the benchmark errors. (If you enable permissions for maintainers to modify the PR then GH offers a button to do this, FWIW)

It shows as enabled for me?

Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, this is a great upgrade to the typing.

There are a couple of small errors, we can def merge when those are solved. Thank you @headtr1ck !

xarray/backends/api.py Show resolved Hide resolved
xarray/backends/api.py Outdated Show resolved Hide resolved
xarray/core/dataarray.py Outdated Show resolved Hide resolved
@headtr1ck
Copy link
Collaborator Author

Puh, 1k LOC diff, that was quite the rabbit hole...

There is still one open point:
The concat_dim arg is not clear to me what the type is actually allowed.
The Index is only there because the docstring mentions it, in the code it is not checked so this might be actually wrong.
Someone with a better understanding of this part should check this type!

@max-sixty
Copy link
Collaborator

Puh, 1k LOC diff, that was quite the rabbit hole...

Yes!

Re the concat_dim — it looks quite reasonable, though I'm not the expert. I would vote to merge before we get conflicts as I recognize it must have taken a lot of work to get here. I will also add a label and ask someone to have a look at our next dev meeting, though I suspect through time we'll find any improvements through bug reports & tests.

Thank you very much @headtr1ck !

@max-sixty max-sixty merged commit 6b1d97a into pydata:main May 17, 2022
@headtr1ck headtr1ck deleted the open_typing branch May 19, 2022 16:08
dcherian added a commit to dcherian/xarray that referenced this pull request May 20, 2022
* main:
  concatenate docs style (pydata#6621)
  Typing for open_dataset/array/mfdataset and to_netcdf/zarr (pydata#6612)
  {full,zeros,ones}_like typing (pydata#6611)
dcherian added a commit to headtr1ck/xarray that referenced this pull request May 20, 2022
commit 398f1b6
Author: dcherian <deepak@cherian.net>
Date:   Fri May 20 08:47:56 2022 -0600

    Backward compatibility dask

commit bde40e4
Merge: 0783df3 4cae8d0
Author: dcherian <deepak@cherian.net>
Date:   Fri May 20 07:54:48 2022 -0600

    Merge branch 'main' into dask-datetime-to-numeric

    * main:
      concatenate docs style (pydata#6621)
      Typing for open_dataset/array/mfdataset and to_netcdf/zarr (pydata#6612)
      {full,zeros,ones}_like typing (pydata#6611)

commit 0783df3
Merge: 5cff4f1 8de7061
Author: dcherian <deepak@cherian.net>
Date:   Sun May 15 21:03:50 2022 -0600

    Merge branch 'main' into dask-datetime-to-numeric

    * main: (24 commits)
      Fix overflow issue in decode_cf_datetime for dtypes <= np.uint32 (pydata#6598)
      Enable flox in GroupBy and resample (pydata#5734)
      Add setuptools as dependency in ASV benchmark CI (pydata#6609)
      change polyval dim ordering (pydata#6601)
      re-add timedelta support for polyval (pydata#6599)
      Minor Dataset.map docstr clarification (pydata#6595)
      New inline_array kwarg for open_dataset (pydata#6566)
      Fix polyval overloads (pydata#6593)
      Restore old MultiIndex dropping behaviour (pydata#6592)
      [docs] add Dataset.assign_coords example (pydata#6336) (pydata#6558)
      Fix zarr append dtype checks (pydata#6476)
      Add missing space in exception message (pydata#6590)
      Doc Link to accessors list in extending-xarray.rst (pydata#6587)
      Fix Dataset/DataArray.isel with drop=True and scalar DataArray indexes (pydata#6579)
      Add some warnings about rechunking to the docs (pydata#6569)
      [pre-commit.ci] pre-commit autoupdate (pydata#6584)
      terminology.rst: fix link to Unidata's "netcdf_dataset_components" (pydata#6583)
      Allow string formatting of scalar DataArrays (pydata#5981)
      Fix mypy issues & reenable in tests (pydata#6581)
      polyval: Use Horner's algorithm + support chunked inputs (pydata#6548)
      ...

commit 5cff4f1
Merge: dfe200d 6144c61
Author: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
Date:   Sun May 1 15:16:33 2022 -0700

    Merge branch 'main' into dask-datetime-to-numeric

commit dfe200d
Author: dcherian <deepak@cherian.net>
Date:   Sun May 1 11:04:03 2022 -0600

    Minor cleanup

commit 35ed378
Author: dcherian <deepak@cherian.net>
Date:   Sun May 1 10:57:36 2022 -0600

    Support dask arrays in datetime_to_numeric
dcherian added a commit to dcherian/xarray that referenced this pull request Jun 12, 2022
* main: (95 commits)
  Use `zarr` to validate attrs when writing to zarr (pydata#6636)
  Add pre-commit hook to check CITATION.cff (pydata#6658)
  Fix kwargs used for extrapolation in docs (pydata#6639)
  Fix notebooks' HTML links (pydata#6655)
  Doc index update (pydata#6530)
  CFTime support for polyval (pydata#6624)
  Support dask arrays in datetime_to_numeric (pydata#6556)
  [pre-commit.ci] pre-commit autoupdate (pydata#6654)
  0-padded month. (pydata#6653)
  [test-upstream] import `cleanup` fixture from `distributed` (pydata#6650)
  Allow all interp methods in typing (pydata#6647)
  Typing support for custom backends (pydata#6651)
  Improved DataArray typing (pydata#6637)
  Adjust code comments & types from pydata#6638 (pydata#6642)
  Typing of `str` and `dt` accessors (pydata#6641)
  Feature/to dict encoding (pydata#6635)
  fix {full,zeros,ones}_like overloads (pydata#6630)
  Mypy badge (pydata#6626)
  concatenate docs style (pydata#6621)
  Typing for open_dataset/array/mfdataset and to_netcdf/zarr (pydata#6612)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type hints for xr.open_dataset
2 participants