search

astropy / pyregion

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

If a wcs has naxis > 2, call 'sub' method. #87

Closed leejjoon closed 7 years ago

leejjoon commented 7 years ago

This is to address #66. If this (or similar) will be accepted, documentation needs to be updated also.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.05%) to 50.66% when pulling 9403162bb84893bb5adb3c1ddf30aab1030135cf on leejjoon:fix_cube into a0751f739debff3461f1edd9bc2b2a7bb7a7242f on astropy:master.

cdeil commented 7 years ago

@leejjoon - Thanks!

Looks to me like this is the preferred solution for everyone that participated in the discussion in #66 .

So I'm putting this under the v1.2 milestone, i.e the release I'd like to make tomorrow.

@leejjoon - Could you do the docs update here? I think it's these two places where the description need to be changed? https://github.com/astropy/pyregion/pull/81/files#diff-a3dcf29bf7e734cf8c6a03dbce0dea2eR136 https://github.com/astropy/pyregion/pull/81/files#diff-a3dcf29bf7e734cf8c6a03dbce0dea2eR221

cdeil commented 7 years ago

@leejjoon - This is the last outstanding PR before I cut the v1.2 release.

Do you have time to rebase and finish it, or should I?

cdeil commented 7 years ago

After thinking about it some more, I would propose to delay this slightly backward-incompatible change to v2.0 (see #94) and release v1.2 now.

@leejjoon - Thoughts?

leejjoon commented 7 years ago

Given that we will be releasing v2.0 within a month, it seems okay to change it to v2.0. Meanwhile, I will update the docs and do the rebase.

cdeil commented 7 years ago

OK, v1.2 is out the door.

@leejjoon - Could you please rebase and do the docs update (see comment above)?

If you have time, please also add a one-line changelog entry here: https://github.com/astropy/pyregion/blob/master/CHANGES.rst#20-unreleased Most of the time in Astropy we do a one-line description linking to the Github pull request, like this:

Change tag attribute from string to list of strings. [#26]
sargas commented 7 years ago

It turns out #100 accidentally fixed #66 too. Basically, both skycoord_to_pixel and pixel_to_skycoord in astropy.wcs.util have this check already. What about just adding the test?

cdeil commented 7 years ago

@sargas - 👍 Can you also update the change log and the docs example (see my comments above)?

sargas commented 7 years ago

Updated in PR #104, but I'm not sure about https://github.com/astropy/pyregion/pull/81/files#diff-a3dcf29bf7e734cf8c6a03dbce0dea2eR221 . It's still the case that shape is needed if only a Header object is given, right?