Fix FK5 -> Galactic conversion

[astrofrog]

As discussed on the list, our FK5 -> Galactic conversion is not consistent with our FK5 -> FK4 -> Galactic conversion, and is not consistent with other codes. If we set the following attributes on Galactic:

coord.Galactic._ngp_J2000 = coord.FK4NoETerms(ra=192.25*u.degree, dec=27.4*u.degree).transform_to(coord.FK5)
coord.Galactic._lon0_J2000 = coord.Angle(122.931918559, u.degree)

then everything works fine and we agree much better with other codes (and are self-consistent). I can't claim to understand all the details and don't have time to look into it more for now, so would be useful to have input from others more familiar with coordinate conversions?

cc @eteq

[astrofrog]

By the way, the angle was determined by optimization, but I think the biggest difference is from the new NGP coordinates. I don't understand why it should be FK4NoETerms, but it just is.

[eteq]

@astrofrog - The first one makes sense to me if the goal is just consistency between the two (and it's as "right" as Reid & Brunthaler 2004, presumably, given the lack of definitive definitions.) As for why it's FK4NoETerms, I think it's because we decided the original definitions should not assume E-terms. This is probably wrong given the strictest definitions (that the original paper was "in FK4", which should have E-terms), but if this is more consistent with other codes, that's far more important than being pedantic at the sub-arcsecond level.

And as you surmised on-list, the lon0 in Reid & Brunthaler 2004 had only has 3 digits, so that explains the problem there. I don't understand why the NGP isn't right, though, as it should be good to milliarcsecs... Anyway, this is fine, again in the name of consistency.

Just so I understand, though, how did you derive the lon0? Did you just find what angle minimizes the FK4->Galactic and FK5->Galactic discrepancy?

Do you want to make a PR to address this, or should I (either way is fine, just don't want us to both do the same thing...)?

[sargas]

Thank you everyone for looking into this issue. I think http://arxiv.org/abs/1010.3773 looks like an approachable source of clarification, although I haven't read much into it.

[astrofrog]

@eteq - yes I wrote a simple optimization code to minimize the discrepancy. I can turn this into a PR.

[eteq]

Thanks for that @sargas - I remember seeing that at some point, but it just served to re-inforce my "Galactic is just not well-defined" perspective. I'm 100% with the point of that paper that it should be based on ICRS, but unless that happens formally I think we're better off just trying to be consistent with what other people do, and leave precision astrometry to ICRS, anyway...

But it might be useful to know if their transforms from ICRS are consistent with the ones we have here. If so, that might be another "shortcut" to implement. It should be pretty straightforward to implement, so that might be a good issue for our bank of issues to point newcomers to?

[eteq]

@astrofrog - Ok, cool, yeah this sounds like a fine solution then.

[astrofrog]

I've attached code to this issue and I think it should be included in 0.4.3.

Regarding that paper - since it's not accepted as far as I can tell, I would suggest that we first discuss it with other code maintainers. If we all agree to adopt this, it might push for publication of that paper/acceptance by the IAU?

[astrofrog]

@sargas - can you confirm that this fixes the issues you were having?

[sargas]

👍
Looks like the coordinate comparisons in pyregion tests are now working with astropy

[astrofrog]

@eteq - can you review this?

[embray]

Have we tried putting # doctest: +FLOAT_CMP on this test?

[astrofrog]

Done

[eteq]

@astrofrog - looks good to me except for the super-minor comment above and @embray's suggestion of using FLOAT_CMP.

If you're busy/traveling and don't think you can get to this soon I can also do a manual merge and implement take care of those things myself.

[astrofrog]

Let's see if Travis passes now with the FLOAT_CMP change

[astrofrog]

@sargas - this is now merged, so you should be able to proceed with the pyregion changes!

[sargas]

@astrofrog - Sweet, thank you. I'll make another comment on astropy/pyregion#54 when I understand enough of ds9's tcl/c/yacc code to find how they handle the angles.