Skip update of naxisj if the slice for dimension j contains non integers - #5455

Unless this is what happened before #5411, I don't think we should ever carry on without actually setting to something. Instead, we should mimic previous behaviour (presumably casting or rounding to int) and raise a deprecation warning (note that numpy went through the same, so people may be slightly used to it...).

@mhvk But one can slice the WCS using floats, at least the computations work correctly. It's just the naxis attribute that can (and should) only be sliced with integers.

cc @nden @keflavich

@nden Any thoughts on this?

So there are two things here: slicing the WCS (which can be sliced with floats) and slicing the data array.
By definition NAXISn represents the number of elements in the data array, i.e. regardless of the WCS slice it can only be integer. In this case does it make sense to compute the new NAXISn using a special rounding method taking into account that the center of an element is 0.5.
Or am I misunderstanding the issue?

Exactly, if the WCS is associated with some data array one needs integers to slice it. In that case the NASXISj values are correctly updated.

In case the slice contains non-integers it just assumes there is no corresponding data array and does not slice the NAXISj values.
It wouldn't make sense to use some rounding here because arrays cannot be sliced with floats, so it makes no sense to slice the WCS as if it was associated with some data array.

In #5411 I unintentionally made it impossible to slice the WCS with float-slices. This PR enables this again but in case the slice contains floats it will not alter the NAXISj values.

But one can slice the WCS using floats, at least the computations work correctly. It's just the naxis attribute that can (and should) only be sliced with integers.

I really don't think we should be supporting slicing WCS with floats as a feature. I don't understand what it means to slice WCS with floats and not modify NAXISj. What is the use case for that?

@astrofrog If we drop it (because it was possible before) it needs a deprecation period where it has to be re-enabled, right?

I can't shrink my image anymore! See #5522.

@MSeifert04 - yes I agree a deprecation phase would be needed. Numpy is doing this (deprecating floating point indices), so I think we should also follow suite and note allow floating point slicing (unless someone can give me a compelling use case)

One thing I'm confused about is that I thought it used to only be possible to slice with step == 1 - at what point did we allow step != 1? The problem is that if I slice an array with ::3 then I'm not really shrinking it but sub-sampling it, so the WCS should be many discontinuous pixels. (I remember discussing this with @keflavich)

In fact our documentation supports that we shouldn't support slices (see slice).

            The ``step`` method, the third argument to a slice, is not
            presently supported.

Anyway, I guess the ship has sailed, but the short of it is that in my view, slicing should be restricted to integer values and we should not allow the step to be !=1, because anything else is ambiguous. We could then have methods for zooming for example (e.g. WCS.zoom).

At the end of the day, I'll leave it up to people who actually use this more than me, so you can just disregard this :)

One relevant comment for this PR though: I think this is more evidence that we need to separate the main WCS class which should nothing about NAXIS from a subclass that would have information about image shape (NAXIS etc), since the behavior for slicing is different (cc @nden)

Apologies for late responses, there's a collision of deadlines around here.
I also want to state the obvious fact that the documentation says step in slices is not supported.
It seems though this was not enforced in the code. @pllim 's example looks like a hack to implement zooming. Is this really the best interface for zooming? It doesn't look intuitive to me at all but I am not a user of this feature and I haven't thought about it too much. As @astrofrog suggested I'd prefer to see a zoom method. I'm also interested in hearing what actual users of the feature think because I don't think it's too late to enforce the "no step in slice" rule (and consider the current behaviour a bug).

Is there another use case for using floats for slicing? On the actual issue here: Is there a reason why we can't modify NAXISn and round them to the nearest integer so to not loose data? If using floats we will always have to deal with this at some level (whether we truncate or pad the image).

👍 on zoom method

@nden @astrofrog

I'm not trying to make float start, stop, step work, neither correct nor incorrect, if it's going to be deprecated or removed when a zoom method and/or #5461 require it. Slicing is really complicated but for integers they already solved it with the builtin range, that's what made it so easy to fix in #5411. Making floating point rounding/truncating would yield a lot of one-off errors when not done carefully - and I don't think that effort is worthwhile when you plan (in other places) to remove it for good soonish.

This PR is simply that if float start, stop or step are given it won't raise an Exception (breaks backwards-compatibility without deprecation) but it will revert to the old behaviour of not modifying naxisj and issuing a Warning (so people are aware that something may not be right!).

Ok, so it sounds like the way forward is to merge this but then also do a PR today or tomorrow that adds a warning (Numpy-style) that non-float slicing will be deprecated in future? I don't know if we have time to add a zoom method before the feature freeze, but we can open an issue for that for the next version?

Ok, so it sounds like the way forward is to merge this but then also do a PR today or tomorrow that adds a warning (Numpy-style) that non-float slicing will be deprecated in future? I don't know if we have time to add a zoom method before the feature freeze, but we can open an issue for that for the next version?

Broadly yes, I would wait until other functionality correctly implements WCS-slicing (zoom) or #5461 is in draft before deprecating it. Otherwise it's just a deprecation without alternative.

At least I use some "slicing WCS with floats" (mostly because of shift) and I don't want to rely on a deprecated feature.

But that's up to you (and the other maintainers) to decide! And this has taken long enough already, so I'll support anything to get this PR in 😄

Maybe a pending deprecation warning would make sense in that case?

What's a pending deprecation? Does it mean: "This is going to be deprecated in the future"?

@MSeifert04 That would be my guess. Something like "we are thinking of deprecating it but there's no replacement yet." pending is a boolean parameter on the deprecated decorator.

@MSeifert04 Can I merge this or do you plan to add something?

@nden Sorry I missed your earlier comment. A pending deprecation should be fine.

I assume the deprecation shouldn't be backported? If so, the PR is ready from my side.