Skip to content

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

Merged
DanRyanIrish merged 5 commits into
sunpy:mainfrom
samaloney:feat-crop-keepdims
Jun 21, 2024
Merged

Add a keepdims kwarg to crop and crop_by_value to keep length-1 dimensions#732
DanRyanIrish merged 5 commits into
sunpy:mainfrom
samaloney:feat-crop-keepdims

Conversation

@samaloney

@samaloney samaloney commented Jun 20, 2024

Copy link
Copy Markdown
Member

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

@DanRyanIrish DanRyanIrish left a comment

Copy link
Copy Markdown
Member

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.

Comment thread ndcube/utils/cube.py Outdated
@DanRyanIrish

DanRyanIrish commented Jun 20, 2024

Copy link
Copy Markdown
Member

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 force-pushed the feat-crop-keepdims branch from bae65a7 to df6f9e6 Compare June 20, 2024 09:24
@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
Comment thread ndcube/ndcube.py Outdated
Comment thread ndcube/ndcube.py Outdated
Comment thread ndcube/utils/cube.py Outdated
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
@DanRyanIrish

Copy link
Copy Markdown
Member

Are you happy for me to merge this @samaloney ?

@Cadair Cadair left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That was pleasantly easy

Comment thread ndcube/ndcube.py
Comment thread ndcube/ndcube.py Outdated
Comment thread ndcube/ndcube.py Outdated
@Cadair

Cadair commented Jun 20, 2024

Copy link
Copy Markdown
Member

We should add narrative docs as well?

@DanRyanIrish

Copy link
Copy Markdown
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>
Comment thread ndcube/ndcube.py
@DanRyanIrish DanRyanIrish merged commit 0107c5d into sunpy:main Jun 21, 2024
@DanRyanIrish

Copy link
Copy Markdown
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.

4 participants