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.

Sign up for GitHub

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

Jump to bottom

Add parse_dims func #7051

Merged
merged 16 commits into from
Nov 30, 2022
Merged

Add parse_dims func #7051

merged 16 commits into from
Nov 30, 2022

Conversation

headtr1ck
Copy link
Collaborator

This PR adds a utils.parse_dims function for parsing one or more dimensions.
Currently every function that accepts multiple dimensions does this by itself.

I decided to first see if it would be useful to centralize the dimension parsing and collect inputs before adding it to other functions.

max-sixty
max-sixty approved these changes Sep 20, 2022
View reviewed changes
xarray/core/utils.py Outdated Show resolved Hide resolved
xarray/core/utils.py Show resolved Hide resolved
@headtr1ck
Copy link
Collaborator Author

I might have thought both of them could be encapsulated in a single func... Maybe that's infix_dims, or fine to rename!

I always thought that these two methods are incompatible.
But I guess ... Is synonym for None (= all dims) and if ... Appears in an iterable, it just means replace it with all leftover dims.

@max-sixty
Copy link
Collaborator

I always thought that these two methods are incompatible. But I guess ... Is synonym for None (= all dims) and if ... Appears in an iterable, it just means replace it with all leftover dims.

Yes! ... is the better synonym — None is somewhat an artifact of history. So +1 for replace_none, which maybe we can gradually turn to False by default over time.

@headtr1ck
Copy link
Collaborator Author

Yes! ... is the better synonym — None is somewhat an artifact of history. So +1 for replace_none, which maybe we can gradually turn to False by default over time.

Nothing wrong with None, it is just pythons default.

The intention of replace_none=False was to leave None as None, which is important for some low level functions as they might be optimized (like numpy sum, which sums over all axes for None).

@max-sixty
Copy link
Collaborator

Nothing wrong with None, it is just pythons default.

The intention of replace_none=False was to leave None as None, which is important for some low level functions as they might be optimized (like numpy sum, which sums over all axes for None).

Not relevant to this PR but for background — it used to be that da.groupby() defaulted to None, and reduced all dimensions — now for a couple of years we require ....

But still da.sum() is equivalent to da.sum(...), which is arguably a bit incongruent with da.sum('a') reducing fewer dimensions than da.sum(['a','b']). But would be quite hard to change now.

@mathause
Copy link
Collaborator

For xr.dot it's also different:

xr.dot(a, b, dims=None) # reduce over common dimensions
xr.dot(a, b, dims=...) # reduce over all dimensions

@headtr1ck
Copy link
Collaborator Author

I think it is best to implement two functions, one for sets of dims and one for sequences of dims. This will be easier to type/read/use than trying to put both kinds into one function.

See also #7094

Maybe parse_dims and parse_ordered_dims?

max-sixty
max-sixty approved these changes Oct 4, 2022
View reviewed changes
xarray/core/types.py Show resolved Hide resolved
@headtr1ck headtr1ck added topic-internals plan to merge Final call for comments labels Oct 26, 2022
Illviljan
Illviljan approved these changes Nov 26, 2022
View reviewed changes
xarray/core/utils.py Show resolved Hide resolved
jhamman and others added 6 commits November 28, 2022 10:56
@jhamman
Merge branch 'main' into parsedims
9e0f312
@mathause @headtr1ck
fix doctests: supress urllib3 warning (pydata#7326)
29673d5
@spencerkclark @pre-commit-ci
@dcherian @headtr1ck
Enable origin and offset arguments in resample (pydata#7284)
91fda37 
* Initial work toward enabling origin and offset arguments in resample

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

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

* Fix _convert_offset_to_timedelta

* Reduce number of tests

* Address initial review comments

* Add more typing information

* Make cftime import lazy

* Fix module_available import and test

* Remove old origin argument

* Add type annotations for resample_cftime.py

* Add None as a possibility for closed and label

* Add what's new entry

* Add missing type annotation

* Delete added line

* Fix typing errors

* Add comment and test for as_timedelta stub

* Remove old code

* [test-upstream]

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Deepak Cherian <[email protected]>
@spencerkclark @headtr1ck
Fix PR number in what’s new (pydata#7331)
23bec8a
@pre-commit-ci @mathause @headtr1ck
[pre-commit.ci] pre-commit autoupdate (pydata#7330)
48f81cc 
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.3.0 → v4.4.0](pre-commit/pre-commit-hooks@v4.3.0...v4.4.0)
- [github.com/PyCQA/autoflake: v1.7.7 → v2.0.0](PyCQA/autoflake@v1.7.7...v2.0.0)
- [github.com/PyCQA/flake8: 5.0.4 → 6.0.0](PyCQA/flake8@5.0.4...6.0.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Mathias Hauser <[email protected]>
@headtr1ck
add comment explaining type: ignore
d3ac8b2
@dcherian dcherian merged commit e4fe194 into pydata:main Nov 30, 2022
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 2, 2022
@dcherian
Merge remote-tracking branch 'upstream/main' into groupby-save-codes-new
004a674 
* 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)
  ...
@headtr1ck headtr1ck deleted the parsedims branch December 8, 2022 20:10
@mgunyho mgunyho mentioned this pull request Aug 19, 2023
Draft
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-sixty max-sixty max-sixty approved these changes

@dcherian dcherian dcherian left review comments

@Illviljan Illviljan Illviljan approved these changes

Assignees
No one assigned
Labels
plan to merge Final call for comments topic-cftime topic-groupby topic-internals topic-typing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@headtr1ck @max-sixty @mathause @dcherian @Illviljan @jhamman @spencerkclark