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

NDCube.squeeze method #669

Merged
merged 9 commits into from
Apr 23, 2024
Merged

NDCube.squeeze method #669

merged 9 commits into from
Apr 23, 2024

Conversation

mihailbankov
Copy link
Contributor

Creates the NDCube.squeeze method as described in the issue, similar to the numpy equivalent.
This method removes (specified) axis with length 1.
Testing for the method is also included.
Resolves #616

Copy link
Member

@DanRyanIrish DanRyanIrish left a comment

Choose a reason for hiding this comment

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

Hi @mihailbankov. Thanks so much for this PR! I've suggested a slight refactor to your implementation that doesn't rely on for loops, which should make the code simpler and more efficient.

Also, I suggest that the an error is raised if a non-length-1 axes tries to be squeezed. This is consistent with np.squeeze which many users will already be familiar with. That change is included in my suggested refactor.

Finally, scalar NDCubes aren't supported because WCS's need non-scalar arrays for their transformations between pixel and world values to make sense. So I've also included an error which is raised if the user tries to squeeze all axes.

Let me know your thoughts. Thanks again!

@DanRyanIrish
Copy link
Member

At first glance, the test coverage does seem a bit weird. I would suggest making the changes to the main code, which will also require some changes to the tests. If the coverage still looks weird, we can take a closer look then.

@mihailbankov
Copy link
Contributor Author

I changed the line for isinstance where I put int; On the last line i changed arr to item; Aside from that I think there are no real changes to the code you proposed. The tests were also changed to test the new code.

@nabobalis nabobalis added this to the 2.3.0 milestone Apr 22, 2024
@nabobalis nabobalis requested a review from DanRyanIrish April 22, 2024 14:31
Copy link
Member

@DanRyanIrish DanRyanIrish left a comment

Choose a reason for hiding this comment

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

We also need to test whether .squeeze() only squeezes the requested axes, rather than all length-1 axes.

@nabobalis nabobalis merged commit d57fcd7 into sunpy:main Apr 23, 2024
19 checks passed
@nabobalis
Copy link
Contributor

Thanks for the PR @mihailbankov

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.

NDCube.squeeze method to slice away length-1 axes
4 participants