sunpy / ndcube

A base package for multi-dimensional contiguous and non-contiguous coordinate-aware arrays. "Maintainers": @danryanirish & @Cadair
http://docs.sunpy.org/projects/ndcube/
BSD 2-Clause "Simplified" License
44 stars 45 forks source link

Allow crop to crop to a single pixel #714

Closed hayesla closed 2 days ago

hayesla commented 1 month ago

Describe the bug

Philosophically this is wrong right? you should be able to crop to a single pixel (@DanRyanIrish says so too ;) )

https://github.com/sunpy/ndcube/blob/main/ndcube/utils/cube.py#L200

related to this https://github.com/sunpy/sunpy/pull/7605

DanRyanIrish commented 1 month ago

The philosophy of crop is that the resulting cube should include all input points. (This is different to slicing, which excludes the upper edge of the interval.) Therefore, it's logically consistent for crop to return a shape-1 cube if the inputs are all in the same pixel. So instead of raising an error, the code quoted above should be something like:

if result_is_scalar:
    item = tuple(slice(idx, idx + 1) for idx in item)

Two things to consider here:

  1. The above special case would mean that all dimensions are kept, they are just length-1. Normally, length-1 dimensions are dropped because it is more convenient to users.
  2. We could institute a rule that, like rebin, crop never drops dimensions. Length-1 dimensions are the norm. This will be more cumbersome in some cases, but the behaviour will be consistent. And now that there is a squeeze() method, it's easy to subsequently drop all length-1 dimensions.

I think either case is viable. It's a philosophical decision. Does anyone have thoughts?

DanRyanIrish commented 2 weeks ago

A compromise to avoid this being a breaking change is to add a keepdims=False kwarg to crop which means no dimensions are dropped. At least this would mean that cropping to a single pixel with keepdims=True would work.

This was decided on the sunpy call that this is the way forward! :)

This the function that needs changing: https://github.com/sunpy/ndcube/blob/main/ndcube/utils/cube.py#L103

At first glance, it may be as simple changing this line to

if keepdims and max_idx - min_idx == 1: