Update "naxisx" attributes when slicing the WCS - #5411

cc: @nden @mcara

@MSeifert04 This looks good to me. Have you looked at the tests? Perhaps add a slicing test or modify it (if there's one).
Ping @keflavich since he implemented slicing.

@nden Thank you!

Yeah, totally forgot about the test. 😅 I modified an existing test.

Did this functionality exist in 1.0.x? If so, this will need to be re-milestoned as 1.0.x and the changelog entry updated.

How do I check that? The 1.0.x files look as if the functionality existed there and it seems the change could be applied there as well. But I just looked at the files, I haven't tried to backport it.

Looks like WCS.slice() was added in 0.4. Are we still supporting 0.x versions?

@nden - no, but we are supporting 1.0.x. @MSeifert04 - can you change the changelog entry to 1.0.11 and we'll change the milestone. Don't worry about whether it backports cleanly, I'll deal with it when doing the actual backporting.

@astrofrog Thank you. I've moved the Changelog to the 1.0.11 section. 👍

Since this is approved, merging.

This PR breaks our code as it forbid setting a value to _naxis1/_naxis2. These variables are private, but there are also the only way to deal with the shape of a WCS object (so I guess this will be an issue for other packages). And it was possible before ;-). Any objection to adding setters for the new properties ? (@nden @MSeifert04)

I didn't add setters because these are private variables and providing explicit setters clearly violates the python principles about variables with one leading underscore - these are implementation details and should never be relied upon.

But I didn't want to break existing code and I wouldn't mind if they get setters 😄 I just never thought about adding these when I made the PR.

@saimn Is it public code that broke?

@nden and I have discussed the idea before of having a sub-class of WCS that does have image size information, since this is a common use case, and maybe that is the one that should support slicing (not the actual WCS class, which could then come completely size-agnostic)

@MSeifert04 - yep, code is here: https://git-cral.univ-lyon1.fr/MUSE/mpdaf/blob/master/lib/mpdaf/obj/coords.py#L590

these are implementation details and should never be relied upon.

In theory yes, but these ones are used everywhere I think, as there is no other way. Another example with a quick search on github: https://github.com/spacetelescope/stwcs/blob/master/stwcs/distortion/utils.py#L58

@astrofrog - It would be nice indeed.

@saimn One workaround would be to use:

wcs = WCS()
wcs._get_naxis({'NAXIS1': n1, 'NAXIS2': n2})

that should also set the naxis attributes but it's just another private method.

I have discussed the idea before of having a sub-class of WCS that does have image size information

@astrofrog That would be 🎉

@MSeifert04 - I can fix the code in many ways, but I prefer if I don't have to fix it ;-). These attributes are used, and there is no gain here to remove this possibility. And no real alternatives (your solution also use a private method and a _get... to set a value ... ).

@saimn A long discussion on the issue of private naxis is here: #4669

In my opinion, there are two distinct approaches: either WCS lives by itself and does not care about the size of the image, or WCS is tied to an image and thus it is natural that a WCS object knows the dimensions of the image. In the later case, I have argued that naxis1 and naxis2 be made public. This is especially important due to the following #5411 (comment) by @saimn :

In theory yes, but these ones are used everywhere I think, as there is no other way.

If so, naxis1 and naxis2 must be made public. It makes no sense in keeping these two attributes private when everybody is relying on them!

@saimn As I said, I have no objections to setters for those attributes, I just haven't realized they were actually used. And the workaround: Just if you need the code working right now.

The change also broke some stuff in astroquery because it cannot handle floats anymore. This definitly needs some regression tests and further patching (or a DeprecationWarning). I'll submit a new PR the next days - if nobody else wants 😄

These used to be public in Astropy FYI and were removed because it was felt the WCS class should be as close to the WCS standard as possible (no image size). But I do believe we need BoundedWCS or something similar, which would be a WCS sub-class with explicit naxis. Then we give people a choice.

@MSeifert04 - No problem, I didn't want to re-launch a huge discussion on WCS 😆 (thanks @mcara for pointing it !), but I just submitted #5454 to add the setters, before going to bed ;).

I kind of like the idea of BoundedWCS by @astrofrog but I also think that could be achieved with the existing WCS class if we:

• make naxis, naxis1, etc. public;
• allow naxis values to be None for "unbounded" WCS;
• provide a convenience property such as bounded to test if a WCS is bounded. Possibly, also provide methods such as bound_to(image) and unbound().

@nden - what is your thought on the best approach - just allow WCS to be optionally bounded to an image, or have a separate class? Either way, I think we should get a solution in for 1.3 and then deprecate the private use of _naxis1/_naxis2.

I am not too enthusiastic about a separate class. If I have to implement the WCS for an instrument I'd always use the one with naxisj because it's good to have them for use in applications. So I see the main WCS class being used very rarely in that case, if ever. I think in this case practicality should trump purity and we should make naxis public again.
I also think the argument for not having them in the main WCS object is too theoretical for a typical user to care about it and having two classes may become confusing. 👍 from me on making them public.