Remove kapteyn_celestial and use astropy when possible

[sargas]

This is a branch not ready for merging due to a dependence on a patched, git version of astropy, but any comments would be appreciated.

Good

• Net loss of 2441 lines, or 56% of origin's LoC going by cloc pyregion/*.py
• Test coverage from 51% to 62%
• test_region in pyregion/tests/test_region.py is a little more usable: reference and test files are parameterized, making it easy to tell what combination failed the test
• Removed estimate_cdelt in favor of astropy.wcs.utils.celestial_pixel_scale
• Some docs for affected methods and new tests
• I think the previous default for how angles are measured was in disagreement with http://cxc.harvard.edu/ciao/ahelp/dmregions.html#Angles . I've made the rot_wrt_axis=2 option from PR Allow selection of the axis for angle measurement to the North #34 the way to go. This only changes things for WCS with non-orthogonal axes.
• Tried to not introduce any new PEP8 violations, and removed a couple.

Bad

• Depends on git version of astropy (celestial_pixel_scale, SkyCoord.to_pixel, etc). I think I'll end up with some _backported_astropy.py submodule for these
• Breaks API, in ways that were not documented (I.e., everything in pyregion.wcs_helper) and by dropping rot_wrt_axis parameter. The latter doesn't come up when Googling at all in any non-pyregion code, though.
• To be useful, needs consistency with DS9 / AST. Currently it's able to do that with Kapteyn. For this PR to be useful, astropy needs Fix FK5 -> Galactic conversion astropy#3107

Ugly

• I'm not convinced the approach in estimate_angle is correct or clear, but I don't have any fits files with non-orthogonal axes to test with. This is exactly @mcara's approach
• pyregion.physical_coordinate looks ripe for gutting and replacing with some standard WCS functions, but I don't see anything in astropy.wcs.WCS that deals with physical coordinates. Perhaps these are called something different in the WCS code?
• There's a lot of files in pyregion/tests/examples that were unused by the tests. I load them as a basic will-this-crash test, but I need to look into these further to see if there is anything special that can be done with them in the tests

[astrofrog]

@sargas - thanks for all your work on this! Let's try and push ahead with the fix to the astropy coordinates. I think we can then release a version of pyregion after astropy 1.0 is out that requires astropy 1.0.

[mcara]

I just want to mention a few things for your consideration. Some may be true but some may be a result of my own mis-understanding, so use with caution.

1. You have said: "I think the previous default for how angles are measured was in disagreement with http://cxc.harvard.edu/ciao/ahelp/dmregions.html#Angles . I've made the rot_wrt_axis=2 option from PR Allow selection of the axis for angle measurement to the North #34 the way to go. This only changes things for WCS with non-orthogonal axes." But the link you are referring to, I think, gives the definition of the rotation angle of a region in the image coordinate system (CS) but not in the tangent plane's CS. In the image's CS axes are orthogonal, but this may not be true in the tangent plane (it will depend on the CD matrix). I think the issue is how does one orient an image to the "North": a) by aligning Y axis (of the tangent plane) parallel to the North or, b) by making X axis perpendicular to the North direction. My understanding is that for HST images this is done by aligning Y || North (case a)). Maybe Chandra uses a different convention, or maybe ground-based telescopes have a different convention from HST. For this reason, and also to keep compatibility with the previous default of pyregion, I allowed users to choose whatever convention they want. Personally, I need rot_wrt_axis=2 for pyregion to produce correctly oriented regions (compatible with DS9) in HST images. I think this issue needs further research in the sense that I am not sure that aligning Y-axis || North is a universally used convention (across all telescopes).
2. I do not know why astropy chose to compute pixel scale the way they do now in celestial_pixel_scale() (maybe for some historical reasons) but it is not the best method (in my opinion). For example, when pixel scales are not equal, celestial_pixel_scale() returns their arithmetic mean. However, I think it should be sqrt(abs(det(CD))). That is, I think that CD matrix is a kind of a Jacobian of the transformation from the image CS to the tangent plane's CS. If that is the case, then area element should be the determinant of the Jacobian and the average pixel scale should be square root of the area element. Let's take a simple case when you have a rectangle of sides a and b. Then the area of the rectangle is a*b. So, if I wanted to replace a and b with a single number, then I would use the side of a square that has the same area as the initial rectangle: sqrt(a*b), that is, I would use for the very least a geometric mean of the two scales and not the arithmetic mean as astropy currently does. With this choice scale**2 will give correct area (on the other hand, what is the meaning of (a+b)/2 as related to scale or area???) Now, what if we have a parallelogram instead of a rectangle (that is, when sides are oblique)? In that case its area is the cross product of the "scaled" vectors along each side - and this is what det(CD) gives (and I hope I did not make a mistake here).
3. If you need an image with non-orthogonal axes, you can use any HST/ACS image, especially from the WFC. (ACS/WFC images, I think, have the most visible effect). You can find them by searching the HST archive for ACS/WFC images. These are typically very large images (about 160MB). A reduced size image can be found in astropy/wcs/tests/data/j94f05bgq_flt.fits. In this FITS file the image has been reduced to a very small size. You can use astropy.io.fits to replace the small image with the original size data array (2048x4096 - I think) in extensions 1-6.
4. You say: "I'm not convinced the approach in estimate_angle is correct or clear, but I don't have any fits files with non-orthogonal axes to test with." I did check that the original estimate_angle (when rot_wrt_axis=2) was giving angles in very good agreement with DS9 in HST images. So, I think you should check that your new function gives the same results as the old one.

[sargas]

@mcara - Thank you, honestly, I think you understand this stuff a lot better then myself. I didn't want to push this until I had the fix in for your files.

1. I think you are right: that document actually describes CIAO region files, which is constrained to physical coordinates. Is there an easy way to tell if someone uses a different convention? I'm just concerned about the mentality of "try it, flip this bit if you notice a problem" as advice for people who don't know the convention of the image they're opening. I think it might be worth just picking whatever ds9 does. Given your experience, I suspect it's the rot_wrt_axis=2 behavior, unless there is some fits keyword for it.
2. That seems reasonable. A lot of my motivation for pushing things off to astropy is the sense that a module that parses region files, and converts to image coordinates on occasion, shouldn't worry about the intricacies of that conversion. I'd recommend filing an issue over at astropy. @astrofrog might have some more guidance on the choices in that method, being involved in More WCS convenience functions: pixel_scale, celestial astropy#2832 and Continued changes to new pixel scale approach by @keflavich aplpy/aplpy#211.
3. Ah, I actually work on NICMOS and WFC3 images; the region files I use are just circles/annuli though. Thanks for the pointers.
4. I'm sorry if I sounded offending, I'm not disputing that the code in estimate_angle works (before or after this PR, it does the same as your approach with rot_wrt_axis=2 with a different cdelt). It's just my confusion about measuring from Y axis (or North) vs counter-clockwise from East (or X axis) and what this function should be. If it depends on a convention that's not specified in the fits header or wcs, then correctness is probably too much to hope for.

[mcara]

@sargas - Unfortunately, I do not think I know that much about WCS - many things are just conventions used by different telescopes so much depends on experience working with images from different telescopes (which I do not have).

Let me address/clarify issues/statements point-by-point:

1. "I think it might be worth just picking whatever ds9 does." I agree with this! Maybe I am wrong, but I think the idea of pyregion (at least as applicable to my needs/usage) is that user "somehow" (likely using DS9) can create some regions using sky coordinates and then I need to convert these regions to image coordinates. The converted (to image coordinates) regions, when loaded back into DS9, should look exactly the same as the original regions (in sky coordinates). Unfortunately this was not true in pyregion until I introduced rot_wrt_axis and set it to 2 (for HST images!!!). I have no idea if any other telescope uses a different convention. If you know for sure that DS9 uses the same convention for images from all other telescopes, then it is OK to assume that rot_wrt_axis=2 always. All I wanted to say is that this may not be the case but I simply do not know that and I do not know why the original code (before I introduced rot_wrt_axis) was actually working as if rot_wrt_axis=1. Was there a reason for that? Maybe some people do not use DS9 but some other program that aligns X to East and not Y to North? For those users, in order to get correctly rotated regions, rot_wrt_axis must be set to 1. Again, I simply do not know what other programs do. Therefore, in order not to break previous pyregion behavior and just to be safe that other users that need to use X->East convention can still get things done in pyregion - I let the users choose the desired behavior by setting rot_wrt_axis to either 2 or 1. However, if you know for sure that rot_wrt_axis=2 is what all people use - then indeed this can be hardcoded. I know that rot_wrt_axis=2 is what I need but I am not sure that this is what everybody else needs.

2. In principle, you are correct about raising this with astopy if this was an omission in the original PR that introduced celestial_pixel_scale() and not because of some historical convention. However, considering the principle that I stated in the previous point of having regions created by pyregion (in image coordinates) to look identical (or as close as possible) to input regions (in sky coordinates) when loaded in DS9 (or other programs), pyregion should either use whatever convention DS9 is using or the most correct estimate of the pixel scale regardless of what celestial_pixel_scale() is computing (unless it is exactly what DS9 does). I just looked at the old (non-astropy) code:

   def estimate_cdelt(wcs_proj, x0, y0): #, sky_to_sky):
   lon0, lat0 = wcs_proj.toworld(([x0], [y0]))
   
   if lat0 == 90 or lat0 == -90:
       raise ValueError("estimate_cdelt does not work at poles.")
   
   lon_ref = lon0 - 180.
   
   lon1, lat1 = wcs_proj.toworld(([x0+1], [y0]))
   lon1 = fix_lon(lon1, lon_ref)
   dlon = (lon1[0]-lon0[0])*np.cos(lat0[0]/180.*np.pi)
   dlat = (lat1[0]-lat0[0])
   cd1 = (dlon**2 + dlat**2)**.5
   
   lon2, lat2 = wcs_proj.toworld(([x0], [y0+1]))
   lon2 = fix_lon(lon2, lon_ref)
   dlon = (lon2[0]-lon0[0])*np.cos(lat0[0]/180.*np.pi)
   dlat = (lat2[0]-lat0[0])
   cd2 = (dlon**2 + dlat**2)**.5
   
   return ((cd1*cd2)**.5)

   and I see the following significant differences:

   • "average" scale was computed using geometric mean and not as an arithmetic mean as does celestial_pixel_scale().
   • Plate scale was computed near the point of interest (x0,y0) because scale is changing in the distorted images, while celestial_pixel_scale() always returns the pixel scale at CRPIX.

3. NICMOS, to the best of my knowledge, has the smallest distortions among all HST instruments and ACS/WFC has the largest. So, to make it easy to visually test/check the effects of the axis choice (for rotations) I strongly suggest that you use ACS/WFC for testing. Of course with circular regions it is impossible to notice any problems 😄

4. I am not offended at all (and I am very sorry if you felt that I felt offended! 😄 ) I simply interpreted (probably incorrectly) your statement "I'm not convinced the approach in estimate_angle is correct or clear" as though you were not sure that the (new) code is correct. For this reason I was simply suggesting that you check it (the new version) against the old code - which, by the way, was not written by me - I simply checked the old code against DS9 to produce reasonable results (but I did not do any thorough checks of the old code and/or used formulae).

[astrofrog]

@mcara - just a quick note on 2) (the celestial_pixel_scale). There is no historical background to this and we just implemented it recently (in fact it's not in any stable release of astropy yet) so please do raise this issue with the astropy core package and we can discuss it there.

[sargas]

So I think I have ds9's implementation (really, saotk) understood and how it differs from what is in the latest commit:

• the offset is set to be the pixel scale to the north
• y_axis_rot is the same if cdelt's in both dimensions are increasing/decreasing and ctypes are in the normal order. Ditto for opposite order (like dec-ra) and cdelts increase in different directions.
• Other combinations of cdelt signs and ctypes cause y_axis_rot - 90 to be negated
• If we are converting to a celestial coordinate system and the east is counter-clockwise of north (e.g., north up and east left, formally np.cross(north_direction, east_direction) < 0)), then the same angle of y_axis_rot + angle - 90. is returned. Likewise if we are not converting to celestial coordinate system and east is clockwise of north.
• If we are converting to a celestial coordinate system xor east is counter-clockwise of north, then y_axis_rot - angle + 90 is returned.

Some things to note:

• For a FITS file with axes in the traditional places, the code agrees with ds9 as written.
• Pixel scale in each dimension would be useful in celestial_pixel_scale() could return a "better" estimate of scale for non-square pixels astropy#3113
• Switching CTYPE[12] order should be noted as unsupported for now (since other parts fail in that case, for instance celestial_pixel_scale). I don't see how this would have worked before, so it isn't a regression. This means we still need to handle the case of y_axis_rot changing if new_wcs.get_cdelt() has different signs.
• The handed-ness convention of coordinate conversions only matter if we are changing the convention. Note that we know the set of coordinate systems used in region files, so the only cases to handle are:
  1. From a left-handed region file to right-handed image coordinates. Almost want to rule this out, but I guess world region file -> rotated WCS might make this possible.
  2. From right-handed region files to left-handed image. The only right-handed coordinates in regions are physical ones, which are handled in a separate code-path not touched by this PR (physical_coordinates.py and convert_physical_to_imagecoord in wcs_converter.py)

I've been figuring out how ds9/saotk/wcssubs does this to compare the algorithm with our own, so I don't think tweaks of this PR to match the algorithm will be a derived work of the GPL code (I haven't written any code yet for that reason). My next steps is getting some exotic headers in the tests, and seeing what I can make pass.

[astrofrog]

@sargas - thank you for all your work on this! We should make sure that all your findings above about the assumptions of the code are clearly indicated in comments in the code. Once you've added the exotic headers, let me know and I can do a review of the changes here.

[astrofrog]

Also, for now it might be worthwhile adding backported versions of the astropy WCS utilities that you use in order to get this to pass.

[sargas]

I wanted to give a little update, since I think I've hit a roadblock that'll take some more work to get over and it's been ~ 2 weeks.

The angle transformations in the latest PR I pushed have a flaw: they should be calculated from the origin for consistency with DS9. This probably justifies this being a helper function here instead of built-in to astropy (regardless of what I intended with astropy/astropy#3093 ).
Right now, distances are transformed by multiplying by some uniform pixel scale. Really, how the distance is transformed should depend on the angle by which it is defined (i.e., if it is a vertical or horizontal distance, for non-square pixels). DS9 uses the x and y cdelts, as well as placeholder points, to do these length transformations. I believe this is what is preventing me from passing tests with a header based on astropy/wcs/tests/data/j94f05bgq_flt.fits, as this is significant for high resolution images with non-square pixels.

I'm not sure of a clean way to do this correctly with the current approach of iterating over coordinates/distances/angles. I'm going to try an approach where Shape objects know how to transform themselves, but it's going to be an even more invasive PR.

[astrofrog]

@sargas - thanks for the update and for working on this! Very much appreciated :)

[cdeil]

I know this is very difficult, but @sargas, is there a way you can bring this PR to a state where it can be merged?
As long as this big PR is open it doesn't make much sense for someone else to start working on pyregion, because there would be duplication of effort and merge conflicts.

[sargas]

@cdeil I've taken a step back from trying to basically rewrite pyregion for now, and focused on doing the transformation correctly.

I updated astropy_helpers in order to make sure I didn't break docs/ (although there's no listing of the API AFAIK).

Updates to .travis.yml were required to make this build again (including dropping numpy 1.5). I'm unsure the cause of the latest build.

[cdeil]

The build failed because of a conda Cython install issue:

Error: HTTPError: 503 Server Error: Service Unavailable: Back-end server is at capacity for url: https://repo.continuum.io/pkgs/free/linux-64/cython-0.23.4-py27_0.tar.bz2: https://repo.continuum.io/pkgs/free/linux-64/cython-0.23.4-py27_0.tar.bz2

https://travis-ci.org/astropy/pyregion/jobs/96339140#L496

I haven't seen this before, maybe @astrofrog can just restart the build now or tomorrow to try again ... sounds like a temp server problem.

[coveralls]

Coverage increased (+12.8%) to 63.482% when pulling c9a0999 on sargas:remove-wcs into 50dde73 on astropy:master.

[sargas]

Just a heads up, this PR should be ready for review (although I know efforts have shifted to astropy/regions)

[cdeil]

@sargas - Thanks!

@astrofrog - How do you want to handle this?

I guess having an improved pyregion is still useful, if only to compare and extract test cases for astropy.regions?

[astrofrog]

@cdeil - I agree!

[cdeil]

I had a quick look at the diff, but I don't have the WCS background and time to look in detail at the changes in pyregion/wcs_converter.py and pyregion/wcs_helper.py.

@astrofrog - Do you have time in the near future or should we try to find someone else?

[astrofrog]

@cdeil - I don't have time to review this in the next couple of weeks (but could do so after)

[bsipocz]

This needs a rebase and a review (cc @cdeil, @astrofrog @keflavich).

[cdeil]

As discussed in #74, we should make a new release of pyregion (see e.g. #55, #61 #64).

My suggestion would be that we do it now, except @sargas or anyone else -- if you have time to work on this PR in the near future, we could wait.

Thoughts?

[sargas]

Rebased. Biggest API change is the lack of rot_wrt_axis parameter (I just use DS9's convention). All tests pass locally on python3.5 and python2.7.

[coveralls]

Coverage increased (+0.1%) to 63.386% when pulling 80c8535 on sargas:remove-wcs into 9c07e37 on astropy:master.

[cdeil]

@sargas - Thanks!

Does anyone have time to review this in the near future?
Are there any major concerns with this, or can this be merged after 1.2, for 2.0?

Clearly, getting rid of Kapteyn and using Astropy more is a huge win, it means that this package is easier to maintain and can much more easily be packaged e.g. for Debian.

Obviously, splitting this into several smaller pull requests would make review much easier. But maybe we just do it and merge this soon, and then iron out any potential issues in master in the coming weeks?

[sargas]

I think this PR has a few things scattered in it:

1. Code cleanups/removals, started in Code cleanup #51; mostly done to try not to leave any pyflakes complaints as I went along
2. More tests, including non-square headers
3. Removal of kapteyn and replacement with astropy-based code.
4. Rotation angle is calculated differently, as if rot_wrt_axis=2. I removed the option for anything else since I couldn't pass all the tests (using the image coordinates from DS9) with the algorithm for rot_wrt_axis=1. This is discussed in Change rot_wrt_axis default? #89 as well.

Should I redo this PR to try to separate these better? As is, it addresses #89, #35, #76, #44, #43, so it is a bit ambitious

[bsipocz]

@cdeil @sargas - The second option seems more optimal (merging it as on PR and fix things later, rather than brake up to smaller pieces that may depend on each other, etc.), but it's up to the people who will do the code review.

[cdeil]

@sargas - Since no-one else is volunteering, I'll review this PR in the coming days, even though I'm not a DS9 or pyregion expert by any means.

If it's not too much work, could you please split out the backwards-incompatible rot_wrt_axis change into a separate PR and put a separate changelog entry for this PR and the rot_wrt_axis one in the 2.0 release section?
http://pyregion.readthedocs.io/en/latest/changelog.html#unreleased

[cdeil]

Personally I almost always call Python functions / methods by keyword argument, as I find the resulting code more readable and less brittle for libraries where I might change the order of arguments.

So changing the argument from sky_to_image(l)to sky_to_image(shapelist) could break my or other user's scripts. Please point out such (slightly) backward-incompatible changes in the changelog, or, if it's not too much work, split them out into a separate PR that is dedicated to those argument name cleanup changes and has a dedicated changelog entry in the 2.0 section.

I would prefer shape_list everywhere over shapelist, but I don't feel strongly about this, no need to argue if you prefer shapelist.

[sargas]

I'll add to the Changelog. I made this change because the 1-3 letter variables make it harder to understand what the code is doing. The l parameter, the class for this function, and the module itself isn't documented, so I don't think there was any intention to make this a public API. Nice to see that regions uses __all__ pretty judiciously to make this stuff clear.

[cdeil]

Ah, sorry, I didn't realise this was on an internal method.
Then I agree, no need to point it out in the changelog (unless you've done it already, in this case OK to keep also).

I just remembered that when adding the API docs I also was tempted to clean up argument names for consistency. E.g. here
http://pyregion.readthedocs.io/en/latest/api/pyregion.get_mask.html it's called region instead of shape_list.
But we can defer this cleanup to separate issues / PRs, this one is mainly about Kapteyn -> Astropy and already very large.

[cdeil]

@sargas - I've left some inline comments.

This mostly looks good to me, I think it should go in soon.
As I said, I'd prefer if the backwards-incompatible changes are split out into separate PRs with separate changelog entries, but the most important thing is that the changes are documented in the changelog at all (which for this package I prefer you do in the PR instead of me having to add it later after merging).

With such a big change, there's bound to be some accidental changes or breaks of user scripts. Increasing test coverage in this PR or one before merging this would probably help, if you have time:
https://coveralls.io/builds/7350828

[cdeil]

@sargas - The https://github.com/astropy/pyregion/blob/master/docs/figures/region_drawing.py script currently emits this warning:

WARNING: AstropyDeprecationWarning: The ascard function is deprecated and may be removed in a future version.
        Use the `.cards` attribute instead. [pyregion.wcs_helper]

Inspired by machete-mode debugging, I added

class MyW(Warning):
    def __init__(self, *args, **kwargs):
        raise 1/0

import astropy.utils.exceptions as aue
aue.AstropyDeprecationWarning=MyW

to find out that the culprit is this Header.ascard call in wcs_helper.py :

Traceback (most recent call last):
  File "region_drawing.py", line 45, in <module>
    r = pyregion.open(reg_name).as_imagecoord(f_xray[0].header)
  File "/Users/deil/Library/Python/3.5/lib/python/site-packages/pyregion/__init__.py", line 57, in as_imagecoord
    shape_list, comment_list = zip(*list(r))
  File "/Users/deil/Library/Python/3.5/lib/python/site-packages/pyregion/ds9_region_parser.py", line 179, in sky_to_image
    wcs_proj = get_kapteyn_projection(header)
  File "/Users/deil/Library/Python/3.5/lib/python/site-packages/pyregion/wcs_helper.py", line 545, in get_kapteyn_projection
    projection = ProjectionPywcsNd(header)
  File "/Users/deil/Library/Python/3.5/lib/python/site-packages/pyregion/wcs_helper.py", line 270, in __init__
    if hasattr(header, 'ascard'):

I think you're not removing or fixing that yet in this PR?
Can you do it or should I file a separate issue?

[sargas]

That was fixed almost 2 years ago in https://github.com/astropy/pyregion/pull/47/files#diff-46f90f66256f758de5bbd1dd5d7a6636L270 ?

[cdeil]

@sargas Thanks, you're right!
That's embarassing ... I had a mix of very old and new files for pyregion in my site-packages.

[cdeil]

This PR has been superseded by #100 .
Closing it now.

@sargas If there's other changes here that should still go in, please make new PRs.

Everyone: please try out master for you applications and file issues if you see any.
See http://pyregion.readthedocs.io/en/latest/changelog.html#unreleased though.