More WCS convenience functions: pixel_scale, celestial - #2832

cc @astrofrog too

To any: are there cases I might be missing? i.e., are there occasions in which pc should be checked?

EDIT: There are errors here - this only works if the pixels are aligned with the grid at the moment

I have mixed feelings about this - strictly speaking the pixel scale changes over the image with the exception of the CAR projection. In addition, it is only really meaningful for WCS with celestial axes. I wonder whether this could not be a function rather than a method on the class (I find it more acceptable to have a function that only works in very specific instances compared to a method, which makes it seem more general).

Good points @astrofrog, I'll address these comments. I am a bit tired of having to recalculate pixel scale "from scratch" all the time; for the vast majority of my work the square-pixel approximation is perfect. But, I concede the point that the pixel size is not uniform, so a warning will be included.

However, I must be writing my test incorrectly, because I get a failure for a filled CD matrix when trying the aplpy approach.

@astrofrog ready for final review

@keflavich - will review this later today, but what do you think of my suggestion of moving these into utils rather than have them be methods of WCS? Also, I think we should maybe rename get_pixel_scale to get_celestial_pixel_scale since it only really applies to those projections, right?

I don't like the idea of moving these to utils - especially celestial is pointless unless it's an attribute, because it's just a very slim wrapper around wcs.sub, but I think it is a more intuitive interface for those not familiar with the wcslib internals. get_pixel_scale I think should be kept in the same location as get_cdelt since it's a comparable concept.

However, renaming sounds appropriate - it's a bit verbose, but that's OK. get_plate_scale would also be appropriate, but I think that terminology is CCD-specific.

@keflavich what about get_celestial_scale and drop the pixel?

@astrofrog - that sounds good. Might be worth bouncing it off one or two others... just ask someone down the hall what they think get_celestial_scale means?

@keflavich - I was thinking about this some more and I really do think that while celestial is fine in WCS, get_celestial_scale is specific enough that it really does belong in the utils as a standalone function. I think it would make sense to have get_pixel_scale in WCS if it did actually work for any WCS. Having said that, I think that the logic of the matrix multiplication (before you do the squaring stuff) could belong in a more general method on WCS and could work with any WCS. What do you think? So basically having a way of returning that final matrix so we don't have to mess around with pc, cdelt, etc.

That last bit is a pretty good idea, and easy to implement - what should we call it? wcs.cd_matrix? wcs.transform_matrix? I like the latter for semantically distinguishing it from the FITS WCS keywords.

I guess I can move get_celestial_scale to utils. I should probably just rename it celestial_scale in that case, since get implies a class method?

@keflavich - not sure if I'd call it cd_matrix since there's already get_cd, right? (but that doesn't work when there's no actual CD matrix?). What about pixel_scale_matrix.

+1 to celestial_scale for the utils function.

Added the matrix. No time for tests now.

@nden - since you are familiar with the WCS code, do you have any thoughts on this pull request so far?

What would be nice in future would be to have a way of computing the pixel scale at a specific position. This would then make it easy to check how much the pixel scale is changing over the image and emit a warning if needed.

It looks good to me. I think it makes sense to name the function reference_celestial_scale or something similar which clarifies the intention and get rid of the warning.
Out of curiosity, why not return both scale values and issue a warning if they are different instead of raise an error? (I don't know how this is intended to be used but for HST, for examle, it will always raise an error.)

@astrofrog - this includes many of the changes from aplpy/aplpy#211 now and is much more flexible.

Can anyone provide a sanity check on the test parameters?

Bump - this is ready for review.

Cool!