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

Switch to T_DataArray in .coords #7285

Merged
merged 17 commits into from
Nov 22, 2022

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Nov 13, 2022

.coords were still using DataArray types switch to T_DataArray instead.

Comment on lines +372 to 373
def __getitem__(self, key: Hashable) -> T_DataArray:
return self._data._getitem_coord(key)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

xarray/core/coordinates.py:372: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]
xarray/core/coordinates.py:372: note: Consider using the upper bound "DataArray" instead

I don't really understand this error, why should the method have at least one argument with the same TypeVar? The TypeVar is stored in self already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what I meant with making coordinates a generic class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not following. Feel free to push to this PR or link me to an example you think is similar.

Copy link
Collaborator

Choose a reason for hiding this comment

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

done :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, nvmd. This does not seem to work in python 3.8....
Anyone has an idea how to solve this? haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it work though? What does a reveal_type(da.coords) give?
Or a reveal_type(da.coords["x"])?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get this on main:

da = xr.DataArray()
reveal_type(da)  # note: Revealed type is "Any"

Do you as well?

Copy link
Collaborator

@headtr1ck headtr1ck Nov 20, 2022

Choose a reason for hiding this comment

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

No, I get note: Revealed type is "xarray.core.dataarray.DataArray" as expected.
(python 3.9.13 and mypy 0.990)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I use:

mypy                      0.982           py310h8d17308_0    conda-forge
mypy_extensions           0.4.3           py310h5588dad_5    conda-forge
...
python                    3.10.6          h9a09f29_0_cpython    conda-forge

@Illviljan Illviljan marked this pull request as ready for review November 20, 2022 09:13
@github-actions github-actions bot added Automation Github bots, testing workflows, release automation and removed topic-typing labels Nov 20, 2022
@Illviljan
Copy link
Contributor Author

Mypy errors with python 3.8. Doesn't appear related to this PR at least.

Run python -m mypy --install-types --non-interactive --cobertura-xml-report mypy_report
  python -m mypy --install-types --non-interactive --cobertura-xml-report mypy_report
  shell: /usr/bin/bash -l {0}
  env:
    CONDA_ENV_FILE: ci/requirements/environment.yml
    PYTHON_VERSION: 3.8
    TODAY: 2022-11-20
    MAMBA_ROOT_PREFIX: /home/runner/micromamba-root
    MAMBA_EXE: /home/runner/micromamba-bin/micromamba
Collecting types-PyYAML
  Downloading types_PyYAML-6.0.12.2-py3-none-any.whl (14 kB)
Collecting types-paramiko
  Downloading types_paramiko-2.12.0.1-py3-none-any.whl (32 kB)
Collecting types-pytz
  Downloading types_pytz-2022.6.0.1-py3-none-any.whl (4.7 kB)
Collecting types-setuptools
  Downloading types_setuptools-65.6.0.0-py3-none-any.whl (48 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 48.9/48.9 kB 19.4 MB/s eta 0:00:00
Collecting types-cryptography
  Downloading types_cryptography-3.3.23.2-py3-none-any.whl (30 kB)
Installing collected packages: types-setuptools, types-PyYAML, types-pytz, types-cryptography, types-paramiko
Successfully installed types-PyYAML-6.0.12.2 types-cryptography-3.3.23.2 types-paramiko-2.12.0.1 types-pytz-2022.6.0.1 types-setuptools-65.6.0.0
xarray/core/types.py:73: error: "tuple" is not subscriptable  [misc]
Generated Cobertura report: /home/runner/work/xarray/xarray/mypy_report/cobertura.xml
Installing missing stub packages:
/home/runner/micromamba-root/envs/xarray-tests/bin/python -m pip install types-PyYAML types-paramiko types-pytz types-setuptools

xarray/core/types.py:75: error: "tuple" is not subscriptable  [misc]
xarray/core/types.py:77: error: "tuple" is not subscriptable  [misc]
xarray/core/types.py:79: error: "list" is not subscriptable  [misc]
xarray/core/merge.py:43: error: "tuple" is not subscriptable  [misc]
xarray/core/merge.py:44: error: "tuple" is not subscriptable  [misc]
xarray/core/merge.py:45: error: "tuple" is not subscriptable  [misc]
xarray/backends/api.py:65: error: "dict" is not subscriptable  [misc]

Generated Cobertura report: /home/runner/work/xarray/xarray/mypy_report/cobertura.xml
Found 8 errors in 3 files (checked 140 source files)

@headtr1ck
Copy link
Collaborator

See #6963 for python 3.8

@github-actions github-actions bot removed the Automation Github bots, testing workflows, release automation label Nov 21, 2022
@headtr1ck headtr1ck added the plan to merge Final call for comments label Nov 21, 2022
@Illviljan Illviljan merged commit ff6793d into pydata:main Nov 22, 2022
@headtr1ck
Copy link
Collaborator

Just noticed that now pylance and mypy return "Any" as Value types.
Maybe it is ok to wait for dropped python 3.8 support...

dcherian added a commit to dcherian/xarray that referenced this pull request Dec 2, 2022
* upstream/main: (39 commits)
  Support the new compression argument in netCDF4 > 1.6.0 (pydata#6981)
  Remove setuptools-scm-git-archive, require setuptools-scm>=7 (pydata#7253)
  Fix mypy failures (pydata#7343)
  Docs: add example of writing and reading groups to netcdf (pydata#7338)
  Reset file pointer to 0 when reading file stream (pydata#7304)
  Enable mypy warn unused ignores (pydata#7335)
  Optimize some copying (pydata#7209)
  Add parse_dims func (pydata#7051)
  Fix coordinate attr handling in `xr.where(..., keep_attrs=True)` (pydata#7229)
  Remove code used to support h5py<2.10.0 (pydata#7334)
  [pre-commit.ci] pre-commit autoupdate (pydata#7330)
  Fix PR number in what’s new (pydata#7331)
  Enable `origin` and `offset` arguments in `resample` (pydata#7284)
  fix doctests: supress urllib3 warning (pydata#7326)
  fix flake8 config (pydata#7321)
  implement Zarr v3 spec support (pydata#6475)
  Fix polyval overloads (pydata#7315)
  deprecate pynio backend (pydata#7301)
  mypy - Remove some ignored packages and modules (pydata#7319)
  Switch to T_DataArray in .coords (pydata#7285)
  ...
@Illviljan Illviljan mentioned this pull request Jan 20, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments topic-typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants