Skip to content

More support of non-string dimension names #2373

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

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

fujiisoup
Copy link
Member

  • Tests passed (for all non-documentation changes)

Following to #2174

In some methods, consistency of the dictionary arguments and keyword arguments are checked twice in Dataset and Variable.
Can we change the API of Variable so that it does not take kwargs-type argument for dimension names?

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.

Thanks a lot @fujiisoup !

"""
Rolling window object.

Parameters
----------
dim: dict, optional
Mapping from the dimension name to create the rolling iterator
along (e.g. `time`) to its moging window size.
Copy link
Collaborator

Choose a reason for hiding this comment

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

small typo!

@@ -290,19 +290,22 @@ def _calc_assign_results(self, kwargs):
results[k] = v
return results

def assign_coords(self, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

I would actually keep this one as before. It’s designed to mirror ‘assign‘, which also only accepts ‘**kwargs‘.

@@ -650,6 +654,8 @@ def resample(self, freq=None, dim=None, how=None, skipna=None,

.. [1] http://pandas.pydata.org/pandas-docs/stable/timeseries.html#offset-aliases
"""
# TODO support non-string indexer after deprecating the old API.
Copy link
Member

Choose a reason for hiding this comment

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

Deprecating -> removing

@shoyer
Copy link
Member

shoyer commented Aug 18, 2018

Can we change the API of Variable so that it does not take kwargs-type argument for dimension names?

I am also tempted to do this, but it’s probably safer not to.

@fujiisoup fujiisoup merged commit 725bd57 into pydata:master Aug 20, 2018
@fujiisoup
Copy link
Member Author

Thanks @max-sixty and @shoyer for your kind review.

@fujiisoup fujiisoup deleted the more_nonstring_dims branch August 20, 2018 01:13
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.

3 participants