search

astropy / pyregion

ds9 region parser for python
https://pyregion.readthedocs.io
MIT License
39 stars 38 forks source link

Remove kapteyn_celestial, Take 2 #100

Closed sargas closed 7 years ago

sargas commented 7 years ago

Here's a simplification of #54, broken up into three commits:

  1. The removal of kapteyn.
  2. Using Astropy 1.0's pixel scale functions for converting lengths/sizes, and changing the algorithm for finding angles change between sky and pixel units. The later is needed for agreement with DS9 with newer tests.
  3. More tests, including with a non-orthagonal axes.

This still breaks backwards compatibility and removes rot_wrt_axis, but at least documents both in the CHANGES file. The tests with files starting with "test03_*" will fail if rot_wrt_axis=1.

cdeil commented 7 years ago

@sargas - Thank you!

Looks good to me.

We should merge this soon to avoid the fate of #54 where it @sargas had to rebase many times and it didn't get merged for years.

@astrofrog @leejjoon @keflavich - Do you have time to review?

If I don't hear back or only get 👍 , I'll merge tomorrow and then there will still be a few weeks where people can review and try out the code in master to shake out potential issues.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 63.13% when pulling 18d0cbd50908ee2c836fb97644caea4c4a9fb589 on sargas:remove-wcs-try2 into cb1254fcf548c4c6219414def23a6657dc3b3326 on astropy:master.

cdeil commented 7 years ago

Merging this now.

Thank you @sargas for all the work and your patience with getting it merged!

leejjoon commented 7 years ago

@sargas : can you elaborate why the header object is required and not the wcs object? My guess from the committed change is to access the value of NAXIS1 and NAXIS2. Do we need more information?

I am asking because I would like to bring back the wcs support. One of the reason is the inefficiency of converting header to wcs object for every shape, which I don't think is necessary. Also, I guess a wcs object created from a header would have a "_naxis[12]" attribute. My inclination is that we allow wcs objects but add optional keyword argument (e.g., shape?) in case _naxis[12] is not defined. Any thought?

mcara commented 6 years ago

@leejjoon I support your request to bring back WCS support. Unfortunately my desire to allow a more accurate coordinate conversion (if so desired) expressed in https://github.com/astropy/pyregion/pull/97#issuecomment-239192187 was ignored.

@SaraOgaz This [removal of support for WCS and full distortion support] makes it difficult to recommend use of astropy/pyregion in its current form in our code. We should continue relying on the earlier version of pyregion.