Add WCS keyword to provide WCS directly.

[gbrammer]

I added wcs keywords to methods throughout to allow specification of a WCS directly, rather than generating the wcs from a passed header. This should make it straightforward for pyregion to parse regions specified in celestial coordinates for images with lookup-table distortion, e.g., Hubble ACS and WFC3/UVIS.

If the wcs keyword is not specified, then behavior in all cases defaults to the current paradigm of generating the WCS directly from the header object as necessary.

[coveralls]

Coverage increased (+0.2%) to 63.759% when pulling 46b320630a076f804d0e7d13acdf6e1fc16171e9 on gbrammer:master into fc3b97941a70440eb3f371348c79010cf05d83ab on astropy:master.

[mcara]

I am not really picky about this and, if my proposal would slow down this PR - forget about it, but I would suggest that we simply get rid of the header thing. A WCS is significantly more versatile than a header. I do not think it is worth having a complicated API for the sake of saving people from crating a WCS object. In addition, for may HST instruments, a WCS created from a header only (as opposite to an HDUList) would produce less accurate results.

That was just an idea and if people do not like it - just ignore this. I need this PR ASAP: unfortunately drizzlepac was made to rely on this astropy/pyregion instead of our own fork and now code crashes with this version due to API changes but I can't make drizzlepac use this pyregion due to the lack of support for user providing their own WCS https://github.com/spacetelescope/drizzlepac/issues/163 (I don't understand why support for WCS was removed in the first place in favor of less capable header).

[bsipocz]

@mcara - out of curiosity, is https://github.com/astropy/regions/ still lacking features you need in drizzlepac? The shared understanding is that continuing development on regions is happening there, and pyregions will get slowly deprecated.

cc @keflavich @cdeil

[mcara]

@bsipocz I didn't know you started working on an alternative package. I can't seem to find anything that would suggest this new package can load/parse/write DS9 region files (and, why not, other formats). Did I miss something?

[keflavich]

@mcara regions is under active development at the moment. It has been able to parse and write ds9 regions for a few years now. New region formats are being added by a GSOC student.

[keflavich]

Also, there is documentation on the ds9 io in regions: http://astropy-regions.readthedocs.io/en/latest/ds9.html so, yes, you did miss something. Please submit a PR if you think there's a better way to make that clear!

[mcara]

The comment in the second point in https://github.com/astropy/pyregion/blob/master/CHANGES.rst#api-changes is strange:

ShapeList.as_imagecoord no longer accepts a asropy.wcs.WCS object. The conversion from pixel to image coordinates depends on the center of the image defined in astropy.io.fits.Header in order to agree with DS9.

How can conversion from pixel to image coordinates depend on the center of the image??? Is this about IRAF's Lterm?

@gbrammer How does this PR handle pixel->image conversion?

[mcara]

@keflavich I just clicked on Getting started which does not mention region files at all. I think this feature one of the main features that I am after in such a package. Therefore, since you asked how this can be improved, I would suggest at least mentioning parsing region files in the "Getting started" section AND moving "Reading/writing to DS9 region files" section up in the "User Guide" (though, I must admit, in a hurry I stopped and clicked on the first link that I hoped would provide an overview of the package - I should have kept reading all the links).

[gbrammer]

@mcara, my change does nothing but accept and pass around an astropy.wcs.WCS object as a parameter rather than generating it on-the-fly from a header. I should say, for exactly the reasons you mention above related to more complicated WCS that aren't self-contained within a header alone.

[mcara]

@gbrammer I know. And it is fine for my purpose. I just do not understand the comment in the changelog and I have no idea what "pixel->image conversion" is and how could we take care of it if only a WCS is provided (though, for the purpose of the drizzlepac, only "image" coordinates are supported - not the "logical" ones that are using IRAF's Lterm if that is what "pixel->image conversion" is about).

[gbrammer]

Ok, I see @mcara. I didn't update the Changelog, so I'm not sure what that is referring to.

The only place where my PR actually does anything is here. I should say that my addition there doesn't do any checking that the supplied WCS is roughly consistent with the header also supplied to convert_to_imagecoord, but otherwise there's no additional accounting for pixel/image conversion, whatever that means, beyond what the module already does.

[mcara]

@gbrammer Is wcs an alternative to the header or is header still required?

[cdeil]

@leejjoon - What do you think about this new option to give a wcs?

@gbrammer - Is this compatible / equivalent to what DS9 does? Can you add a test case where the world to pix conversion was done with DS9, and you assert that with this feature pyregion does the same?

[mcara]

It seems that in the past header could also be a WCS object. In https://github.com/astropy/pyregion/pull/100#issuecomment-301568614 @leejjoon wanted to restore that functionality. Maybe it should be restored as it was instead of adding another parameter that may conflict with header (i.e., when both are provided).

[leejjoon]

I guess the support for the wcs has been dropped by #100. The issue was that, to calculate the angle which is consistent with the ds9 convention, we need to know the center of the image (which is calculated by NAXIS1 and NAXIS2). And my impression was that WCS does not have a public API for NAXIS?.

https://github.com/astropy/pyregion/blob/master/pyregion/wcs_helper.py#L30

Given this, I think it would be better to have an explicit "wcs" keyword argument. On the other hand, I think we also need to check whether the header argument is a WCS object for the backward compatibility.

• if wcs is None and header is WCS object: create a dummy header with NAXIS1/2 and use it as a header. We can use WCS._naxis[12] attribute for this. If these attributes are not defined, just raise an error.