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

adding stack_all #3115

Closed
wants to merge 8 commits into from
Closed

adding stack_all #3115

wants to merge 8 commits into from

Conversation

vrx-
Copy link

@vrx- vrx- commented Jul 13, 2019

I'm working on issue #1029. Will try to add a test today. Suggestions welcome

rabernat and others added 3 commits July 12, 2019 23:52
* switching out examples to use nbsphinx

* added jupyter_client to doc env

* added ipykernel to doc env
* switching out examples to use nbsphinx

* added jupyter_client to doc env

* moved gallery to notebook
@codecov
Copy link

codecov bot commented Jul 13, 2019

Codecov Report

Merging #3115 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3115      +/-   ##
==========================================
- Coverage   95.99%   95.97%   -0.02%     
==========================================
  Files          63       63              
  Lines       12796    12808      +12     
==========================================
+ Hits        12283    12292       +9     
- Misses        513      516       +3

@pep8speaks
Copy link

pep8speaks commented Jul 13, 2019

Hello @vrx-! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1304:9: F841 local variable 'result' is assigned to but never used

Comment last updated at 2019-07-17 01:40:06 UTC

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @vrx- . I've added some comments.

EDIT: You should add a line under "New functions" in whats-new.rst and give yourself credit.

xarray/core/dataarray.py Outdated Show resolved Hide resolved
xarray/core/dataarray.py Show resolved Hide resolved

Parameters
----------
name : Name of the new dimension. Optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion as above.

* x (x) |S1 'a' 'b'
* y (y) int64 0 1 2
>>> stacked = arr.stack_all()
>>> stacked.indexes['z']
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

DataArray.stack
DataArray.unstack
"""
result = self
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need self.copy()?

xarray/tests/test_variable.py Outdated Show resolved Hide resolved
@@ -1341,6 +1341,13 @@ def test_stack(self):
expected = Variable(('X', 'Y'), v.data, v.attrs)
assert_identical(actual, expected)

def test_stack_all(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add similar tests to test_dataarray.py and test_dataset.py

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.

Looks good! Thanks @vrx. I echo @dcherian 's suggestions, and added one tiny change

xarray/core/variable.py Outdated Show resolved Hide resolved
xarray/core/variable.py Outdated Show resolved Hide resolved
@@ -1280,6 +1280,30 @@ def stack(self, dimensions=None, **dimensions_kwargs):
result = result._stack_once(dims, new_dim)
return result

def stack_all(self, name='stacked'):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need the xarray.Variable method.

Variable is a lower-level API, mostly for internal use inside xarray to implement functionality for DataArray and Dataset. If we aren't using a Variable method as part of the implementation for Dataset/DataArray, then that is a good clue that it may not be necessary.

vrx- and others added 3 commits July 16, 2019 14:12
Co-Authored-By: Deepak Cherian <dcherian@users.noreply.github.com>
Co-Authored-By: Stephan Hoyer <shoyer@gmail.com>
Co-Authored-By: Deepak Cherian <dcherian@users.noreply.github.com>
@dcherian
Copy link
Contributor

Hi @vrx- do you have time to pick this up again?

@DancingQuanta
Copy link

Would it be possible to add exclude_dims? I have a use case where I wanted to apply a function (for example fitting) along a dimension. I programmatically create a list of dimensions to stack to create 2D dataset. Then I loop over the stacked dimension and apply the function to last dimension.

@mathause
Copy link
Collaborator

#3826 enables da.stack(stacked=[...]) to stack all dimensions.

Can this also be closed?

@max-sixty
Copy link
Collaborator

#3826 enables da.stack(stacked=[...]) to stack all dimensions.

Can this also be closed?

I think so.

But @vrx- you made a great start here, and we'd love you to become a contributor in the future!

@max-sixty max-sixty closed this Mar 25, 2020
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.

8 participants