Use distortion-aware WCS transformations

[mcara]

This addresses issue #35

[coveralls]

Coverage remained the same at 63.243% when pulling c7215ed72559e6a64c3061bd9156099fdb84833c on mcara:distortion-aware-wcs-transforms into 9c07e37ddcb229486fc21bc28c26f3539c338ee2 on astropy:master.

[cdeil]

@mcara - Thanks!

This brings pyregion in line with what ds9 does, right?

Do you have time to add a test that covers this? I.e. one example that asserts on something where this change makes a difference?

I'm putting this under the 2.0 milestone as it's a change that affects results, i.e. backwards incompatible, and plan to merge it after having tagged 1.3.

@sargas @keflavich @leejjoon - OK to do this change?

[sargas]

:+1:

PR #54 makes this change as well, accidentally, since SkyCoord.to_pixel and SkyCoord.from_pixel default to using the all_* methods instead of wcs_*.

[mcara]

@cdeil "This brings pyregion in line with what ds9 does, right?"

No, DS9, I think, does not apply distortion corrections. So, I think we are better than DS9. I will think of a test how to check this and I will post back.

[mcara]

Ok, I take back what I said previously. It seems like DS9 is using SIP corrections for region position computations but it does ignore tabular distortions (cpdis and det2im). So, we are still "better" than DS9 (after switching to all_* functions) though these tabular corrections are not that large.

[astrofrog]

So, we are still "better" than DS9

Arguably, we might not want to be different from DS9, so if we are better currently, sounds like we should try and be worse 😆 (because region files will be created in DS9, and we want the Python values to be consistent).

[mcara]

In point no. 1 in https://github.com/astropy/pyregion/pull/54#issuecomment-63265572 I expressed the same idea of pyregion matching DS9 behavior. This looks like a sensible thing to do as long as it is the correct thing to do. Let me provide two examples for and against matching DS9 behavior.

First, let's assume we are dealing with an HST/ACS FLT image (these images contain all kinds of distortions). Let's say we have a region file given in image coordinates. When we load such a file in DS9, DS9 (appears to me) will convert regions to sky coordinates [for the very least we could save regions in DS9 in sky coordinates; in other words we could use DS9 to convert regions from image to sky coordinates]. Now, we could use pyregion to do the same: load the region file given in image coordinates and then convert it to sky coordinates. Ideally, we would want to match DS9 results because if we don't then there will be a mismatch between the regions produced by DS9 and the ones produced by pyregion.

Now, let me give you an example of a case when a more accurate conversion is more important than matching DS9. Often, observed stars/sources are too faint to be reliably identifiable in an FLT image. In order to increase SNR often multiple FLT images are drizzled ("co-added") onto a common rectified grid. I will denote the obtained image as DRZ. These DRZ images do not contain distortions. Now, because SNR is better in DRZ, I will select regions in this DRZ image and then I will load them (or use them to produce masks) into the FLT image. In this case, it is imperative that world coordinates are correctly converted to pixel coordinates if we want to produce masks as close as possible to the original regions selected in the DRZ image. DS9 would produce inaccurate results in this case.

I do not think we can reconcile these two approaches (matching DS9 and producing accurately converted regions). In my opinion it is more important to produce accurate results than matching DS9. In addition, users can control which distortion they leave in a WCS object before passing it to pyregion. This means we will have to document the two usage scenarios and inform them on how to control distortions. On the other hand, if we hard-code pyregion to match DS9 then there will be no way to "add back" distortions omitted by pyregion.

[cdeil]

@mcara - Thanks for the detailed comment.

I fully agree that this important point should be documented in the pyregion docs. Explain what ds9 (apparently) is doing. Explain what ds9 is doing by default. Add at least one test. Explain how to get a different result from pyregion (e.g. by modifying WCS before passing it in) using an example.

Concerning implementation in pyregion, it's not clear to me yet what the goal and default behaviour should be ("most correct", or "most similar to ds9"), and whether it would be useful enough to expose a choice on method to use as keyword argument in pyregion.

@sargas and @mcara seem to be in favour of doing this change in default behaviour. Is anyone here not in favour of this change?

Personally I don't care, all my data has simple WCS with no distortions. My feeling is that since pyregion is so closely coupled to the DS9 region model, most users probably would expect the default behaviour to match what DS9 does?

[mcara]

@sargas _"PR #54 makes this change as well, accidentally, since SkyCoord.to_pixel and SkyCoord.from_pixel default to using the all* methods instead of wcs*."_

Does this mean this PR is obsolete if PR #54 (or any of its descendants) is accepted?

[cdeil]

@sargas - Can you confirm that this change was made as part of #100 already?

If yes, this PR should be closed.

And the changelog should me updated to explicitly mention this change, so that people won't be surprised if they science results change with the update to pyregion 2.0, no? I think it's not clearly mentioned yet in the changelog, right? http://pyregion.readthedocs.io/en/latest/changelog.html#unreleased

[cdeil]

@sargas - I've added you as pyregion maintainer just now and assigned this issue to you.

If you don't have time, just assign back to me.

[sargas]

I put up a seperate PR commenting on the change in the changelog. If there's no complaints, I'll merge and close this PR.

[astrofrog]

I think this can be closed, so I'll do that now