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

Add a keepdims kwarg to crop and crop_by_value to keep length-1 dimensions #732

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

samaloney
Copy link
Contributor

@samaloney samaloney commented Jun 20, 2024

Add a keep_dims kwarg to crop and crop_by_values to allowing length-1 dimensions to be kept see #714

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.

Thanks for this PR @samaloney. It looks good, but keepdims should also be added to crop_by_values. And a test for that similar to he one you already have.

ndcube/utils/cube.py Outdated Show resolved Hide resolved
@DanRyanIrish
Copy link
Member

DanRyanIrish commented Jun 20, 2024

Additionally, the kwargs should be keepdims to be consistent with numpy, not keep_dims.

@samaloney samaloney marked this pull request as ready for review June 20, 2024 09:20
@samaloney samaloney changed the title Add a keep_dims kwarg to crop function. Add a keepdims kwarg to crop and crop_by_value to keep length-1 dimensions Jun 20, 2024
ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/utils/cube.py Outdated Show resolved Hide resolved
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
@DanRyanIrish
Copy link
Member

Are you happy for me to merge this @samaloney ?

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

That was pleasantly easy

ndcube/ndcube.py Show resolved Hide resolved
ndcube/ndcube.py Outdated Show resolved Hide resolved
ndcube/ndcube.py Outdated Show resolved Hide resolved
@Cadair
Copy link
Member

Cadair commented Jun 20, 2024

We should add narrative docs as well?

@DanRyanIrish
Copy link
Member

We should add narrative docs as well?

It probably would be good to add a couple sentences and a code snippet at the end of this section: https://github.com/sunpy/ndcube/blob/main/docs/explaining_ndcube/slicing.rst#cropping-with-real-world-coordinates
It doesn't have to be much. Something like:

By default, crop and crop_by_values discard length-1 dimensions to make the resulting cube more wieldy.
However, there are cases where it is preferable to keep the number of dimensions the same.
In this case, uses can set the keepdims kwarg to True in either crop or crop_by_values.

(Add code snippet example showing shape of cropped cube with length-1 dimensions.)

One use case for `keepdims=True` is when cropping leads to a cube with only one array element.
Because cropping an `NDCube` to a scalar is not allowed, such an operation would normally raise an error.
But if `keepdims=True`, a valid NDCube is returned with N length-1 dimensions.

(Perhaps another code snippet example.)

Co-authored-by: Stuart Mumford <stuart@cadair.com>
@DanRyanIrish DanRyanIrish merged commit 0107c5d into sunpy:main Jun 21, 2024
15 of 17 checks passed
@DanRyanIrish
Copy link
Member

Thanks @samaloney!!

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.

None yet

4 participants