Allow greater flexibility in crop bounds order

[rosteen]

Describe the feature

I recently noticed while doing some work in specutils (see https://github.com/astropy/specutils/pull/1033/files#r1134253955) that when providing both a sky coordinate and spectral coordinate as bounds to the NDCube crop method, the physical types need to be in the same order as they appear in the WCS. For example, if the spectral axis is last in the WCS, you can do this:

   >>> lower = [SkyCoord(ra=205, dec=26, unit=u.deg), SpectralCoord(4.9, unit=u.um)]
   >>> upper = [SkyCoord(ra=205.5, dec=27.5, unit=u.deg), SpectralCoord(4.9, unit=u.um)]

but not this:

    >>> lower = [SpectralCoord(4.9, unit=u.um), SkyCoord(ra=205, dec=26, unit=u.deg)]
    >>> upper = [SpectralCoord(4.9, unit=u.um), SkyCoord(ra=205.5, dec=27.5, unit=u.deg)]

It seems like it shouldn't matter in what order the SpectralCoord and SkyCoord are provided, since in the WCS it's unambiguous which axes these apply to. I'm also not sure what would happen if you had the spectral axis in the middle of the WCS, (e.g., [RA, Wavelength, Dec]), which seems silly but isn't impossible. I propose adding some logic to make this a little more flexible and avoid the error trace that occurs currently if the bounds are provided out of order (I don't have the trace on hand right now, unfortunately, I can add it here later).

Proposed solution

No response

[DanRyanIrish]

Hi @rosteen. Thanks for this comment. I tried to make this work when we were redesigning crop but the more we thought about I thought about it, the less I was convinced that this could be done in an unambiguous, generalised way. #379 is related to this. If I remember our thinking at the time, we came to the conclusion that a simple implementation of input order sanitisation relied on the assumption that was no repetition of classes world_axis_physical_types, which NDCube cannot guarantee. Given this, the logic you want could quickly become prohibitively complex.

A further complication may be NDCube.crop's support for including None as a valid input type if no cropping along that physical type is needed. This relies on mapping the None to its corresponding physical type via is position in the input.

I can't say that I convinced myself that what you want is definitely impossible, but nor could I prove to myself that it was definitely possible and so we required that the inputs be given in an order that can be transparently be passed to the WCS. But if you are able to prove that this can be unambiguously determined in all cases and can come up with some (preferably) not complicated logic then that would be a great addition to NDCube!

[DanRyanIrish]

Having thought more, I think it should be possible to implement this by first checking if there is repetition of types in the WCS components. If there is repetition then the inputs are passed to the WCS without order sanitisation as is currently done. If there isn't repetition, sanitisation can be performed. In this scenario, it could also be possible to drop the need to include Nones for non-cropped dimensions.

The concern I'd have about this thought is that it would result in different behaviours depending on the underlying WCS and break our policy that all APIs should ideally work for all valid NDCube instances. For example, users who rarely have duplication of world types in their WCS will likely not learn that the input order is important and develop workflows on that basis. Then the same workflow would error if a valid NDCube with world types duplication is passed through it. This may be frustrating and the reason why may not be obvious.
Worse still would be a case when the unordered inputs happen to align with the order of world types in the WCS, but cause the wrong input to be fed to the wrong axis. In this case the function wouldn't error but would give an unexpected result. It might take a lot of digging for a user to figure out what went wrong as the expected result might be achieved with another NDCube without world types duplication.

One solution would be to raise a warning whenever world type duplication is detected and tell the user that the order now matters and possible what the required order is for that WCS. Although raising a warning for valid operations is not best practice and many users will ignore those warnings.

All that said, I think in the majority of cases this functionality would be very convenient.