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

Convert new_shape from list to tuple in _unstack_once #5319

Merged
merged 1 commit into from
May 16, 2021

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented May 16, 2021

Having new_shape as a list broke some checks in sparse. .shape is usually a tuple so I changed new_shape to be a tuple as well. sparse arrays errors one step further down now instead, at the item assignment...

@Illviljan Illviljan closed this May 16, 2021
@Illviljan Illviljan reopened this May 16, 2021
@max-sixty
Copy link
Collaborator

It's so small & reasonable & internal that it's probably fine not to have tests

@max-sixty max-sixty merged commit cef9356 into pydata:master May 16, 2021
@@ -1579,7 +1579,7 @@ def _unstack_once(

# Potentially we could replace `len(other_dims)` with just `-1`
other_dims = [d for d in self.dims if d != dim]
new_shape = list(reordered.shape[: len(other_dims)]) + new_dim_sizes
new_shape = tuple(list(reordered.shape[: len(other_dims)]) + new_dim_sizes)
Copy link
Collaborator

@keewis keewis May 17, 2021

Choose a reason for hiding this comment

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

probably not worth a new PR, but if .shape is always a tuple this could be simplified to

Suggested change
new_shape = tuple(list(reordered.shape[: len(other_dims)]) + new_dim_sizes)
new_shape = reordered.shape[: len(other_dims)] + tuple(new_dim_sizes)

@Illviljan Illviljan deleted the Illviljan-unstack_once_shape branch May 18, 2021 18:14
@Illviljan Illviljan mentioned this pull request Jul 5, 2021
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