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

added methods from xr.Dataset #1254

Merged
merged 29 commits into from
Jul 27, 2020
Merged

Conversation

percygautam
Copy link
Contributor

@percygautam percygautam commented Jun 20, 2020

Description

Related to #1066

Checklist

  • Follows official PR format
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@percygautam
Copy link
Contributor Author

Added the methods: isel, stack, unstack, rename, rename_vars, rename_dims

@percygautam
Copy link
Contributor Author

percygautam commented Jun 20, 2020

Tasks:

  • update 'unstack' method to include dim
  • add tests
  • add more methods

@percygautam
Copy link
Contributor Author

Hi all, I have created the wrapper function for adding xr.Dataset methods:

    def extend_xr_method(func):

        @functools.wraps(func)
        def wrapped(*args, **kwargs):
            _filter = kwargs.pop('filter_groups', None)
            _groups = kwargs.pop('groups', None)
            _inplace = kwargs.pop('inplace', False)
            self = args[0]
            args = list(args)

            out = self if _inplace else deepcopy(self)

            groups = self._group_names(_groups, _filter)
            for group in groups:
                xr_data = getattr(out, group)
                args[0] = xr_data
                xr_data = func(*args, **kwargs)
                setattr(out, group, xr_data)

            return None if _inplace else out

        return wrapped

Please review, so I can implement it.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I have not reviewed everything completely yet, will get to it whenever I have time. There is one issue that needs some discussion between us though, so I'll push for a lab meeting.

There are also several comments on docs which could also be tackled on a follow-up PR if desired.

Comment on lines 449 to 452
for key, value in kwargs.items():
if not set(value).difference(dataset.dims):
kwarg_dict[key] = value
dataset = dataset.stack(**kwarg_dict)
Copy link
Member

Choose a reason for hiding this comment

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

Have not tried yet, so could be completely wrong, please check. I fear that the loop will only work for one of the ways of calling stack (first example), and not the other (second example). I would find this quite confusing. Here are the two examples:

arr = xr.DataArray( 
     np.arange(6).reshape(2, 3), 
     coords=[("x", ["a", "b"]), ("y", [0, 1, 2])]
) 
example_1= arr.stack(z=("x", "y"))
example_2 = arr.stack({"z": ("x", "y")})

Note: I think idata.sel does the same too, so we should probably decide what do want ArviZ to do automatically and if this behaviour of kwargs working automatically vs dicts not working automatically is desired or should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're probably right. I have done the changes for the stack. Should I also correct other methods too, or wait for the lab-meeting?

Copy link
Member

Choose a reason for hiding this comment

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

I'd wait for lab meeting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I don't remember what was decided in the lab-meeting. Is it discussed? What to do in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OriolAbril What about this one?

@percygautam percygautam requested a review from OriolAbril July 6, 2020 17:31
@percygautam
Copy link
Contributor Author

@OriolAbril multiple tests are failing for this PR, can you help?

@OriolAbril
Copy link
Member

I don't have time to fix mpl right now so I temporally pinned the version to <3.3 until we can make ArviZ code compatible with 3.3, I think this should fix the issues and get ci to pass again. see #1305

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I think it's ready to merge, we can polish docs in next PR.

@OriolAbril OriolAbril merged commit 785a0b5 into arviz-devs:master Jul 27, 2020
@percygautam percygautam mentioned this pull request Aug 7, 2020
3 tasks
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