Repository navigation
Finish up Astrometric frame and spherical_offsets_to - #4909
Conversation
…rame of a coordinate
This function didn't really do anything that can't be done by just creating a new frame from the same thing. I.e, frame.__class__(orgin=neworigin) is the same as frame.at_origin(neworigin)
| i00.spherical_offsets_to(fk5) | ||
|
|
||
|
|
||
| #large offset (>1 deg) should give a warning |
There was a problem hiding this comment.
No longer true, so remove this comment!
|
@eteq - this looks great, although the actual machinery is still beyond me (it would be good to have a "coordinates for developpers" section in the documentation, just like @astrofrog has been thinking about a similar one for modelling). |
|
The two remaining failures in travis seem to real (it is a doctest). |
|
Alright, I think I've addressed all of @mhvk's comments. Re:
Good idea! I created #4913 to try to help us remember to do this. I admit there's some real black magic going on in the metaclasses here. Although I confess I enjoyed it: there's some really tricky stuff going on there that is really only possible due to Python's inherent flexibility. |
|
weird... the remaining failure is due to doctests that come out as either |
|
Sounds good to just change the example. I think this reflects a display issue in older versions of numpy -- I'd guess FLOAT_CMP would pick it up if there were a dot after the number (which there should be). |
|
in the end I concluded that it was good to keep one example with 0s (to demonstrate the idea) and just skip that in the doctests. The rest are all non-0 now (and otherwise essentially identical), so it's still sufficiently tested. |
|
@eteq - rather late to this conversation, but I had always assumed that astrometric frames would include a roll (or parallactic) angle. This has practical application as a poor-mans WCS to make a quick sky image (perhaps a finding chart) based on a catalog and telescope parallactic angle. |
|
@taldcroft -Ah, good point. Was pretty easy to include, so the commit I just pushed up includes a One catch with this: it's now somewhat less right to call it "dra" or "ddec" if the rotation is non-0. But I think it's ok to just accept that - it's still reasonably clear, and one can always do |
|
@eteq - awesome on getting in the rotation! I had expected to have to wait until 1.3. 😄 |
|
Oh, one thing about the rotation: I just picked the sign in the way that I thought was the most likely use case ("rotate to match a thing at position angle x"), but I'm really not sure if that's the most intuitive direction. Probably best to just merge this once the tests are passing, and then have an issue about that, as we can gather user opinions during beta. |
|
Turns out there's a really obscure bug in py 3.3: the Anyway, after the last commit (which just adds a compatibility layer to fix up the py 3.3 asdict), we should be good to go. |
|
Looks good! |
|
@eteq - on the names: from what I've seen from, e.g., POSS and UCAC calibration, if one goes for relative positions, sometimes xi and eta are used as local longitude and latitude relative to a plate centre (e.g., the UCAC paper by Zacharias et al. http://adsabs.harvard.edu/abs/2000AJ....120.2131Z). But those of course still have unknown position, so arguably that would be a bit different astrometric frame. Still, it may not be bad to use those for the names, since for large values, As for the rotation, I think one always it to be such that with rot=0, a star to the North from the ref star would have a positive offset in latitude, with rot=90, that would hold for a star in the East, etc., i.e., the rotation defines where a positive offset in latitude goes, from N going through E. |
|
(I've created #4931 to continue the conversation since this is now merged as-is) |
This PR is an amalgamation of related work, particularly from #4338 and #4524 (it replaces both). Thanks to @alexrudy and @Kevtron for their contributions in the latter!
I won't dwell on the ideas too much because they been covered in all the other issues I reference here and have been reviewed piece-meal in the various PRs. So if the tests pass my feeling is that this is all set!
One significant change from #4524 that's worth mentioning though: this now uses some metaclass magic to convince the class generator (which creates astrometric classes) to both give correct class names (previously they were all called
Astrometricwhich causes various issues - now they get names likeAstrometricICRS), and change the spherical components to e.g.draandddec/dlonanddlat(instead ofraanddecorlonandlat). I think this is safer because that makes it immediately clear when reading the code that it's an astrometric ("offset") frame instead of just an ordinary frame.So if merged, this closes #4209, closes #4338, closes #4339, and closes #4524 (whew!)