Switch to T_DataArray and T_Dataset in concat #6784
Conversation
for more information, see https://pre-commit.ci
Line 486 in c190b36 Line 618 in c190b36 Line 654 in c190b36 Slightly annoying errors here. |
|
I think several method were not changed to |
|
The fact that these two doesn't always match each other should affect the user no? Is there any trick to avoid this or should I just close this for now? |
|
It should only affect the user when they use custom subclasses of DataArrays, something that is discouraged anyway. |
Casting |
xarray/core/concat.py
Outdated
| @@ -590,7 +590,7 @@ def get_indexes(name): | |||
| # preserves original variable order | |||
| result_vars[name] = result_vars.pop(name) | |||
|
|
|||
| result = Dataset(result_vars, attrs=result_attrs) | |||
| result = cast(T_Dataset, Dataset(result_vars, attrs=result_attrs)) | |||
There was a problem hiding this comment.
Here the actual result value is hardcoded to be of type Dataset which would not match T_Dataset if datasets contains some subclass of xarray.Dataset.
Suggestion to try (which may not need the cast):
result = type(datasets[0])(result_vars, attrs=result_attrs)
There was a problem hiding this comment.
Nice find! I do wonder if we have this issue in more places?
There was a problem hiding this comment.
Yes this is quite endemic to hardcode Dataset / DataArray, rather than use the class of the argument.
We would need to make some widespread changes for this to work correctly — I think that would be welcome. We'd probably need to find a good way to test this across methods too (though maybe mypy could help there?).
And we might need a property of Dataset for what to class to use when returning a DataArray, which becomes more complicated.
There was a problem hiding this comment.
Looks like this broke somebody's subclass that doesn't support the attrs argument: #6827
* main: (313 commits) Update whats-new Release notes for v2022.06.0 (pydata#6815) Drop multi-indexes when assigning to a multi-indexed variable (pydata#6798) Support NumPy array API (experimental) (pydata#6804) Add cumsum to DatasetGroupBy (pydata#6525) Refactor groupby binary ops code. (pydata#6789) Update DataArray.rename + docu (pydata#6665) Switch to T_DataArray and T_Dataset in concat (pydata#6784) Fix typos found by codespell (pydata#6794) Update groupby attrs tests (pydata#6787) Update map_blocks to use chunksizes property. (pydata#6776) Fix `DataArrayRolling.__iter__` with `center=True` (pydata#6744) [test-upstream] Update flox repo URL (pydata#6780) Move _infer_meta_data and _parse_size to utils (pydata#6779) Make the `sel` error more descriptive when `method` is unset (pydata#6774) Move Rolling tests to their own testing module (pydata#6777) [pre-commit.ci] pre-commit autoupdate (pydata#6773) move da and ds fixtures to conftest.py (pydata#6730) Bump EnricoMi/publish-unit-test-result-action from 1 to 2 (pydata#6770) Type shape methods (pydata#6767) ...
Noticed these issues when trying to add typing to some helper functions in #6778.