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

implement map_over_datasets kwargs #10012

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

kmuehlbauer
Copy link
Contributor

@kmuehlbauer kmuehlbauer commented Jan 31, 2025

@kmuehlbauer kmuehlbauer changed the title Map over datasets kwargs implement map_over_datasets kwargs Jan 31, 2025
Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

This looks good! Though needs some docs, even just the docstring.

Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Thanks. One question on the implementation.

@kmuehlbauer
Copy link
Contributor Author

Though needs some docs, even just the docstring.

Yes, please suggest or add as you see fit.

Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

I guess "Allow kwargs in map_over_datasets" is enough for the changelog entry.


I'll approve. Just want to add, that the old implementation allowed the kwargs to be a DataTree, while this does not (I assume this is a deliberate decision in the re-implementation?). I don't want to hold up this PR for this reason but just remark that this makes using it ever so slightly more annoying, i.e. you may have to re-arrange your inputs to accommodate for it.

https://github.com/xarray-contrib/datatree/blob/5f3956ffe80e686dd3df54ee8cef9ff56c158e76/datatree/mapping.py#L162

@TomNicholas TomNicholas added the topic-DataTree Related to the implementation of a DataTree class label Feb 4, 2025
@TomNicholas
Copy link
Member

the old implementation allowed the kwargs to be a DataTree, while this does not (I assume this is a deliberate decision in the re-implementation?)

The implementation as of this PR matches apply_ufunc, which also doesn't allow you to pass xarray objects as kwargs.

Comment on lines 1449 to 1450
*args : tuple, optional
Positional arguments passed on to `func`.
Copy link
Member

Choose a reason for hiding this comment

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

Should we show / state that the *args can be DataTrees while the kwargs cannot? This is shown clearly in the apply_ufunc docstring.

@kmuehlbauer
Copy link
Contributor Author

@TomNicholas Is this now more explicit in the docstring?

Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Thank you @kmuehlbauer - just one nit, but I'm approving.

@kmuehlbauer kmuehlbauer merged commit 1189240 into pydata:main Feb 10, 2025
29 checks passed
@kmuehlbauer kmuehlbauer deleted the map_over_datasets-kwargs branch February 10, 2025 10:25
@mathause
Copy link
Collaborator

Thanks!

mathause added a commit to mathause/mesmer that referenced this pull request Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-DataTree Related to the implementation of a DataTree class
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow kwargs in map_over_datasets?
3 participants