Repository navigation
Conversation
…hem instances of QuantityFrameAttributes
…rame of a coordinate
…plane and the translation to the galactic center
|
(assigning myself to review it as soon as I get a chance...) |
|
I haven't been following the discussion in all the other issues so far, and it's really long, so apologies if my comments here aren't very helpful. First of all, @Kevtron, thanks for implementing this! I asked @eteq today at PyAstro16 whether "field of view coordinates" are implemented in I think it basically does what I need: a rotation to put a given point at Given a star at fixed RA / DEC and a telescope an fixed pointing RA / DEC, as well as an observer and array of times (within a 1 hour interval), what are the "field of view" coordinates in a system where the pointing position is defined as In pseudo-code, this is what I want: radec_star = SkyCoord(...)
radec_pointing = SkyCoord(...) # field of view center coordinate = pointing position
altaz_frame = AltAz(obs_times, location, ...)
altaz_pointing = radec_star.transform_to(altaz_frame)
fov_radec = radec_star.transform_to(AstrometricFrame(radec_star))
fov_altaz = radec_star.transform_to(AstrometricFrame(altaz_pointing))Does this make sense? Also: @Kevtron, this PR needs to be rebased. And are the examples in the docstring auto-tested? If not, I think they should be re-implemented as unit tests ... when checking against other code it's really useful to have some reference numbers, not just complicated code which re-computes the expected result. |
|
@cdeil That pseudo-code is the use case intention of this feature! I need to update the docstring because I was -very- lazy and copied the implementation directly from galactocentric and stripped out the pieces I didn't need. The docstring was the last on my list of things to do and this fell off my radar. I'll work on those unit tests + updating it to actually accurately explain what it is this actually does. The functional tests are a little complicated and I was trying to do a more general test rather than picking some magic numbers and trying them, but I think I may have shot myself in the foot and gone too far. I'll work on this this week, it sort of fell off my radar. |
|
@Kevtron - Great. Thanks!
Ah, OK, that makes sense. @frame_transform_graph.transform(FunctionTransform, ICRS, Astrometric)
+def icrs_to_astrometric(icrs_coord, astrometric_frame):Is it possible to make it re-usable for other coordinate frames, i.e. specifically to make an AltAz astrometric frame which rotates a given point to Also -- it would be great if you could add a high-level example (like the code snippet I pasted) in the high-level docs and explain a bit what an "astrometric" frame is.
Personally, I'd say yes. Re-implementing the rotation in the test by calling all kinds of trigonometric functions isn't very helpful / makes the test hard to review / maintain. If it were me, I would try to simplify / shorten the tests and remove any re-implementation of methods inside the test functions (even if it comes at the expense of less tests in a certain sense).
Thank you very much! |
|
@Kevtron - one other important detail here: I think this implementation is subtly different from what #4339 was about (and I think also @cdeil's use case): the implementation here is tied to ICRS. The problem with that is that (I think), the astrometric frame then is always aligned with the ICRS axes. @cdeil wants to be able to have the I have some thoughts on how you might do that, but before I delve into that, I want to make sure I'm understanding your implementation right - does the above sound right to you? |
|
It's been quite some time since I've thought about celestial coordinate systems, so I'm a little confused by
My implementation does do the rotations relative to the ra/dec center, which was what I understood from #4339, but is by no means an absolute requirement. Any suggestions on how to make it more usable are definitely welcome! |
|
@Kevtron @eteq Any chance to get this in Astropy 1.2? Would be nice to have this feature. Deadline is in 2 days and I'm on holiday, so I won't be able to help at all with coding. |
|
I ended up implementing something similar to this for some code I'm working on (like Astrometric, but also had to support rotation) and I think I understand what @cdeil is asking for, however I don't think it is trivial in coordinates right now: To make this work with AltAz frames I think we'd need a second Astrometric class which uses AltAz where this one uses ICRS in the transform graph. I'm not sure how to easily do that, especially because an Astrometric frame class tied to AltAz won't convert to ICRS in the same way as one tied to ICRS... |
| default_representation = CartesianRepresentation | ||
|
|
||
| origin_ra = QuantityFrameAttribute(default=0, unit=u.degree) | ||
| origin_dec = QuantityFrameAttribute(default=0, unit=u.degree) |
There was a problem hiding this comment.
Could you use a custom frame attribute here which holds a SkyCoord? That way you could convert the origin to the reference frame (ICRS) automatically for users.
I did this like this:
class CoordinateLocationAttribute(FrameAttribute):
"""A frame attribute which is a coordinates object."""
def convert_input(self, value):
"""
Checks that the input is a SkyCoord with the necessary units (or the
special value ``None``).
Parameters
----------
value : object
Input value to be converted.
Returns
-------
out, converted : correctly-typed object, boolean
Tuple consisting of the correctly-typed object and a boolean which
indicates if conversion was actually performed.
Raises
------
ValueError
If the input is not valid for this attribute.
"""
if value is None:
return None, False
elif isinstance(value, ICRS):
return value, False
else:
if not hasattr(value, 'transform_to'):
raise ValueError('"{0}" was passed into an '
'CoordinateLocationAttribute, but it does not have '
'"transform_to" method'.format(value))
icrsobj = value.transform_to(ICRS)
if isinstance(icrsobj, SkyCoord):
return icrsobj.frame, True
return icrsobj, Trueand then replacing these lines with
origin = CoordinateLocationAttribute()I think this would also make it easier to factor out the base reference frame, if we can figure out how to make the transforms work for arbitrary target frames.
There was a problem hiding this comment.
@alexrudy - yes, that's exactly what I had in mind when first suggesting this.
|
I've been thinking about this on-and-off, and recognized a similar problem to what mentions here for the more "general" version I had in mind (where the "target" frame is set by the), plus one other:
So my conclusion is that there needs to be a distinct class for every frame that wants an Astrometric form. This leaves me with two alternatives:
I will try to take a crack at 2 now(ish), but if that doesn't work out soon I think we should just do 1 for 1.2, and think of 2 as an a possible new feature for 1.3. |
|
@eteq I took a crack this afternoon at 2, but got stumped when I couldn't get my tests to pass in the ICRS case. I can take another look tomorrow if that helps, but I don't want to delay inclusion of this in 1.2 if its going to require a lot of work. |
|
@alexrudy - can you point me to the branch where you did this? Perhaps I can carve out the time to finish it up later today. |
|
@eteq I've got some work here: https://github.com/alexrudy/astropy/tree/astrometric-frame |
|
Oh, great - good thing I asked! I'll try to synthesize that with some of the additional parts done in this branch and make a new PR just for times' sake (but it will have commits from both of you!) |
This is what I have so far for the astrometric frame for #4339. Still TODO are: update the docstring in AstrometricFrame to reflect what it actually does, debug
test_astrometric_ra_dec_functionaland the test for the convenience methodto_astrometric. The "functional" tests are a bit tough to debug if they fail, so I'm probably also going to refactor them in some way also.