Change rot_wrt_axis default?
Closed this issue · 8 comments
In #84 I documented the rot_wrt_axis parameter as suggested by @leejjoon and @mcara .
While doing this I read the following comment by @mcara in PR #34 where he introduced the parameter:
pyregion computes the image rotation angle using as the angle between North and X-axis instead of typically adopted convention of using Y-axis for image orientation computations. This produces regions that are rotated compared to the regions produced by DS9 when image axes are non-orthogonal (at least for HST images).
This sounds to me like the default should have been 2 (y-axis), to be consistent with what ds9 does (always or by default). Currently the default is 1 (x-axis), and if a user doesn't know about or set this option, they get a result different from what DS9 gives?
@mcara @leejjoon @keflavich - Is that correct?
If yes, should we treat this as a bug and change the default for the upcoming 1.2 release (or call that version 2.0 to allow for such braking changes).
For now, I'm putting this under the "whishlist" milestone and tag it "question", i.e. if I don't hear back here or there's no consensus what to do, I'll just release v1.2 as-is and leave this to the future.
Personally, I am not aware of any software that computes rotations with regard to X-axis (except for pyregion). However, I am far from being familiar with all kind of software out there and so my proposal was based on observation of the behavior of DS9 as well as the way orientation angles are being computed in STScI software. [I think I have also found evidence of measuring angles from Y-axis in several other packages but this was so long ago that now I cannot recollect these packages.]
Based on what I know, I think making default rotation axis to be 2 (=Y-axis) would serve most people the best. When I did my original PR #34 I set the default to 1 (X-axis) in order not to break existing applications using pyregion. If most people think that having the default axis with regard to which angles are measured to be Y-axis - I am all for it.
Moreover, if it could be confirmed that the convention (used by most/all people/institutions) is to measure the angle relative to the Y-axis, I would suggest hardcoding this and getting rid of this parameter completely in order to simplify API.
It seems that I have already proposed my previous proposal in #29 (comment)
@leejjoon @keflavich, maybe @larrybradley or @ericmandel - Thoughts?
I'm +1 to treat this as a bug and make the backwards-incompatible change @mcara is proposing to behave like DS9 does (if it is correct that DS9 always uses the Y-axis as reference, which I didn't check myself yet).
+1: Position angle is usually measured relative to the +y axis (e.g. "North up" is PA=0).
if it is correct that DS9 always uses the Y-axis as reference, which I didn't check myself yet
This is correct. The reference originally was the X-axis, but this was changed to Y-axis in the 1990s (except the documentation at: http://js9.si.edu/regions/regions.html was never changed until just now ... Yikes!)
The current default behavior is simply due to my ignorance. And I am happy to see this changed.