Coordinates API, take 2 #12
Changes from 19 commits
5f97986
7272fbf
a9eba4a
c7e6fa7
7e22502
94c830f
7b8872d
e80200b
e1c497b
2636c2e
16daf98
fda9449
f5badff
428be96
20b37b1
b9e80cb
b9b3e43
133e7c2
130de7a
0e85445
3ccefb4
d52e10b
971e2c2
fb46b2c
411da47
4ee8bdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,343 @@ | ||
# -*- coding: utf-8 -*- | ||
|
||
from astropy import coordinates as coords | ||
from astropy import units as u | ||
|
||
#<-----------------Classes for representation of coordinate data---------------> | ||
#These classes inherit from a common base class and internally contain Quantity | ||
#objects, which are arrays (although they may act as scalars, like numpy's | ||
#length-0 "arrays") | ||
|
||
#If `Latitude` or `Longitude` objects are given, order doesn't matter | ||
coords.SphericalRepresentation(coords.Latitude(...), coords.Longitude(...) | ||
|
||
#similar if `Distance` object is given | ||
coords.SphericalRepresentation(coords.Latitude(...), coords.Longitude(...), coords.Distance(...)) | ||
coords.SphericalRepresentation(lat=5*u.deg, lon=8*u.hour) | ||
|
||
#Arrays of any of the inputs are fine | ||
coords.SphericalRepresentation(lat=[5, 6]*u.deg, lon=[8, 9]*u.hour) | ||
|
||
#Default is to copy arrays, but optionally, it can be a reference | ||
coords.SphericalRepresentation(lat=[5, 6]*u.deg, lon=[8, 9]*u.hour, copy=False) | ||
|
||
#strings are parsed by `Latitude` and `Longitude` constructors, so no need to | ||
#implement parsing in the Representation classes | ||
coords.SphericalRepresentation(lat='5rad', lon='2h6m3.3s') | ||
|
||
#Or, you can give `Quantity`s with keywords, and they will be internally | ||
#converted to Angle/Distance | ||
c1 = coords.SphericalRepresentation(lat=5*u.deg, lon=8*u.hour, distance=10*u.kpc) | ||
|
||
# Can always just accept another representation, and that means just copy it | ||
c1 = coords.SphericalRepresentation(c1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I guess we do have to be flexible in the inputs if we want to allow this. I'm just slightly concerned about performance, though this may be premature optimization. One could make the API:
then this example still works, and it's just when we pass lon/lat or distance that we have to be explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I guess I could live with this, but was definitely preferring the option of allowing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In practice, you'll never do:
because you could just do:
There's no reason you would initialize a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course, you may initialize
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess my worry is that you're allowing such flexible input that there's going to be a lot of logic in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eteq - I think that:
would be better because it does allow
but also
I guess I'm advocating a no-magic approach (with the understanding that this can also be changed later), but where the only convenience is that
is also that from the function signature, it's not clear that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fwiw I am 👍 on no-magic. I like @astrofrog's suggestion here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think putting the representation first is rather odd, as it will not be the most common way the routine is called. Surely,
should work without naming the parameters! My sense would be to put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a low level class I agree with @astrofrog in not liking the idea of having There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, in the version I'm pushing up soon, I've opted for |
||
|
||
# distance, lat, and lon typically will just match in shape | ||
coords.SphericalRepresentation(lat=[5, 6]*u.deg, lon=[8, 9]*u.hour, distance=[10, 11]*u.kpc) | ||
#if the inputs are not the same, if possible they will be broadcast following | ||
#numpy's standard broadcasting rules. | ||
c2 = coords.SphericalRepresentation(lat=[5, 6]*u.deg, lon=[8, 9]*u.hour, distance=10*u.kpc) | ||
assert len(c2.distance) == 2 | ||
#when they can't be broadcast, it is a ValueError (same as Numpy) | ||
with raises(ValueError): | ||
c2 = coords.SphericalRepresentation(lat=[5, 6]*u.deg, lon=[8, 9, 10]*u.hour) | ||
|
||
# a `store_as` option is important for some cases - this is basically a | ||
# convinience to make it easier to combine inhomogenous angle array or string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a few more below, so maybe just search and replace. |
||
# lists. See astropy/astropy#1421 for justification | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think #1421 should apply to the low-level classes, since these accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was @mdboom that specifically mentioned wanting it in, and I didn't want that to delay anything given there weren't other opinions at the time, which is why it got added back in. It's definitely less crucial with the string stuff moved to the other classes, but it still might be useful when you want to ensure the internal representation is always the same unit regardless of what @mdboom, do you still think this needs to be in there? |
||
c2 = coords.SphericalRepresentation(lat=[5, 6]*u.deg, lon=[8, 9]*u.hour, store_as=(u.radian, u.radian)) | ||
assert c2.lon.units == u.radian | ||
|
||
#regardless of how input, the `lat` and `lon` come out as angle/distance | ||
assert isinstance(c1.lat, coords.Angle) | ||
assert isinstance(c1.distance, coords.Distance) | ||
|
||
#but they are read-only, as representations are immutible once created | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eteq - I agree with this, but I vaguely recall in earlier discussions that you (or someone) had some reason for wanting mutability in coordinate objects. In general life gets easier if you enforce immutability, e.g. it's easier to cache things as needed for speed, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this started when @ejeschke said this would made ginga work faster, @taldcroft. There is a subtle point here that we are not enforce true immutability at the array level, because one can always do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify, I think it might be possible to modify quantity to be able to set this as read-only, as @taldcroft said in the next comment down... but I think we shouldn't do that because there are some existing cases where this might be a pragmatic case to allow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can set the EDIT: Though obviously document that in a more user-friendly way than I just did ;) |
||
with raises(AttributeError): | ||
c1.lat = coords.Latitude(...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment above (although it has to be |
||
#Note that it is still possible to modify the array in-place, but this is not | ||
#sanctioned by the API, as this would prevent things like caching. | ||
c1.lat[:] = [0] * u.deg # possible, but NOT SUPPORTED | ||
|
||
#To address the fact that there are various other conventions for how spherical | ||
#coordinates are defined, other conventions can be included as new classes. | ||
#Later there may be other conventions that we implement - for now just the | ||
#physics convention, as it is one of the most common cases. | ||
c3 = coords.PhysicistSphericalRepresentation(phi=120*u.deg, theta=85*u.deg, rho=3*u.kpc) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't it be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I should read more, it does say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, either would be just fine - will add a line reflecting that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (added, but it didn't clobber this because I added lines below) |
||
c3 = coords.PhysicistSphericalRepresentation(phi=120*u.deg, theta=85*u.deg, r=3*u.kpc) | ||
with raises(ValueError): | ||
c3 = coords.PhysicistSphericalRepresentation(phi=120*u.deg, theta=85*u.deg, r=3*u.kpc, rho=4*u.kpc) | ||
|
||
#first dimension must be length-3 if a lone `Quantity` is passed in. | ||
c1 = coords.CartesianRepresentation(randn(3, 100) * u.kpc) | ||
assert c1.xyz.shape[0] == 0 | ||
assert c1.unit == u.kpc | ||
#using `c1.xyz.unit` is the same as `c1.unit`, so it's not necessary to do this, | ||
#but it illustrates that `xyz` is a full-fledged `Quantity`. | ||
assert c1.xyz.unit == u.kpc | ||
assert c1.x.shape[0] == 100 | ||
assert c1.y.shape[0] == 100 | ||
assert c1.z.shape[0] == 100 | ||
# can also give each as separate keywords | ||
coords.CartesianRepresentation(x=randn(100)*u.kpc, y=randn(100)*u.kpc, z=randn(100)*u.kpc) | ||
#if the units don't match but are all distances, they will automatically be | ||
#converted to match `x` | ||
xarr, yarr, zarr = randn(3, 100) | ||
c1 = coords.CartesianRepresentation(x=xarr*u.kpc, y=yarr*u.kpc, z=zarr*u.kpc) | ||
c2 = coords.CartesianRepresentation(x=xarr*u.kpc, y=yarr*u.kpc, z=zarr*u.pc) | ||
assert c2.unit == u.kpc | ||
assert c1.z.kpc / 1000 == c2.z.kpc | ||
# although if they are not `Distance` compatible, it's an error | ||
c2 = coords.CartesianRepresentation(x=randn(100)*u.kpc, y=randn(100)*u.kpc, z=randn(100)*u.deg) # raises UnitsError | ||
|
||
# CartesianRepresentation can also accept raw arrays and a `unit` keyword | ||
# instead of having units attached to each of `x`, `y`, and `z`. Note that this | ||
# is *not* the case for other representations - it's only sensible for | ||
# Cartesian, because all of the data axes all have the same unit. | ||
coords.CartesianRepresentation(x=randn(100), y=randn(100), z=randn(100), unit=u.kpc) | ||
|
||
#representations convert into other representations via the `represent_as` method | ||
srep = coords.SphericalRepresentation(lat=0*u.deg, lon=90*u.deg, distance=1*u.pc) | ||
crep = srep.represent_as(coords.CartesianRepresentation) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My reasoning for this is that "transform" is a heavily overloaded word, just like "coordinate system". So I'm trying to use "transform" to mean "change from one reference system/frame to another", but not for transforming representations. It's mostly semantics, but the discussion around this had a lot of confused/overlapping terminology, so I'm trying to push this API in the direction of being rather pedantic. Other opinions on this? @mdboom @astrofrog @Cadair ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the context of generalized WCS my preference is to use "transform" to mean a conversion of co-ordinate values from one system to another. Just my vote FWIW. Of course IRAF users are accustomed to it meaning resampling data... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just so we're on the same page, @jehturner, can you clarify what you mean by "system", here? I think you mean one "reference system" or frame, not representation? (See the APE5 text to see how I'm defining these terms) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel that strongly about this point. In fact the more I think the more I feel like coordinate transformations should really be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like @astrofrog's solution here - that nicely avoids the question, and we want that behavior to always work anyway. Sounds good to you too, @taldcroft ? @jehturner - Just a quick point on this: I'd rather we not write the language of this API in a way that is influenced by a full WCS - I think a lot of astronomers just want a coordinate API for many tasks (e.g. working with catalogs). (although if we adopt @astrofrog's solution, it's a moot point because the wording no longer enters in the API here at all) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For @astrofrog's suggestion to work we are still going to need a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for my opinion, I like both methods, I see different uses for each. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know I'm here late (been at meeting all last week), but 👍 to this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also 👍 on @astrofrog's suggestion. But this begs the question should we remove
Note that this does something potentially good because it provides a natural opportunity to provide additional metadata needed for a coordinate system transformation. There has always been this problem that a generic Caveat - I'm late for lunch and haven't thought this through carefully. 😄 |
||
assert crep.x.value == 0 and crep.y.value == 1 and crep.z.value == 0 | ||
#The functions that actually do the conversion are defined via methods on the | ||
#representation classes. This may later be expanded into a full registerable | ||
#transform graph like the coordinate frames, but initially it will be a simpler | ||
#method system | ||
|
||
|
||
#Future extensions: add additional useful representations like cylindrical or elliptical | ||
|
||
|
||
#<---------------------Reference Frame/"Low-level" classes---------------------> | ||
#The low-level classes have a dual role: they act as specifiers of coordinate | ||
#frames and they *may* also contain data as one of the representation objects, | ||
#in which case they are the actual coordinate objects themselves. | ||
|
||
|
||
#They can always accept a representation as a first argument | ||
icrs = coords.ICRS(coords.SphericalRepresentation(lat=5*u.deg, lon=8*u.hour)) | ||
|
||
#which is stored as the `data` attribute | ||
assert icrs.data.lat == 5*u.deg | ||
assert icrs.data.lon == 8*u.hour | ||
|
||
#Frames that require additional information like equinoxs or obstimes get them | ||
#as keyword parameters to the frame constructor. Where sensible, defaults are | ||
#used. E.g., FK5 is almost always J2000 equinox | ||
fk5 = coords.FK5(coords.SphericalRepresentation(lat=5*u.deg, lon=8*u.hour)) | ||
J2000 = astropy.time.Time('J2000',scale='utc') | ||
fk5_2000 = coords.FK5(coords.SphericalRepresentation(lat=5*u.deg, lon=8*u.hour), equinox=J2000) | ||
assert fk5.equinox == fk5_2000.equionx | ||
|
||
#the information required to specify the frame is immutible | ||
fk5.equinox = J2001 # raises AttributeError | ||
|
||
#As is the representation data. | ||
with raises(AttributeError): | ||
fk5.data = ... | ||
|
||
#There is also a class-level attribute that lists the attributes needed to | ||
#identify the frame. These include attributes like the `equinox` above. | ||
assert FK5.frame_attr_names == ('equinox', 'obstime') # defined on the *class* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how it will work out in practice, but for Partially in that spirit, maybe this could be a (self-referring) dictionary? It could then have a nicer name to, say, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't completely understand what you mean, here, @mhvk: you mean instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second was just meant for
But that would seem an implementation detail. As for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, exactly - My reasoning for this scheme is that we want the frame specifiers to be immutible (at least initially), which is easily implemented as read-only properties, but is much harder to enforce if
I admit that's a bit more awkward than dictionary-style access, though. So @mhvk, do you know of a straigthforward way to implement a "read-only dictionary" backed by attributes like this? If not, this seems the easiest thing to do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eteq - I see your point, the dictionary is perhaps not the best way after all. Just to be sure I understand where this is going to be used: would this be mostly in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mhvk - I guess something like this could go in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My expectation was that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eteq - indeed, I meant that the primary use of it would be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I understand what you meant now, @mhvk - so yes, I agree with @taldcroft that it makes sense to leave it for public consumption. So I think we're now all on the same page. I'll add a note to make it explicit that this is intended to be used in |
||
assert fk5.frame_attr_names == ('equinox', 'obstime') # and hence also in the objects | ||
|
||
#The actual position information is accessed via the representation objects | ||
assert icrs.represent_as(coords.SphericalRepresentation).lat == 5*u.deg | ||
assert icrs.spherical.lat == 5*u.deg # shorthand for the above | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My idea is that each representation class could have an This part may be a detail, but there is a subtle difference from the coordinate frame transforms: it's not a graph. There has to be a direct transform from one representation (or its superclass) to another. Users can register new representation transform functions, and they would follow the same rules as the builtin ones. That has the advantage of being trivial to implement, but if later it turns out we want a full transform graph like for the frame transforms, we can add it in at that time. I didn't want to nail the details of this down in the API, because I think experimenting a bit with actual code and different implementations might help. The high-level class would always use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That all makes sense. I agree we can experiment later with whether this API should only allow direct hops or to automatically traverse a graph of transformations. I don't think the API itself restricts either option, but obviously whatever we choose to do should be documented. |
||
assert icrs.cartesian.z.value > 0 | ||
|
||
#Many frames have a "preferred" representation, the one in which they are | ||
#conventionally described, often with a special name for some of the | ||
#coordinates. E.g., most equatorial coordinate systems are spherical with RA and | ||
#Decoords. This works simply as a shorthand for the longer form above | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: |
||
|
||
assert icrs.ra == 5*u.deg | ||
assert fk5.dec == 8*u.hour | ||
|
||
assert icrs.preferred_representation == coords.SphericalRepresentation | ||
|
||
#low-level classes can also be initialized with the preferred names: | ||
icrs_2 = coords.ICRS(ra=8*u.hour, dec=5*u.deg, distance=1*u.kpc) | ||
assert icrs == icrs2 | ||
|
||
#and these are taken as the default if keywords are not given: | ||
icrs_nokwarg = coords.ICRS(8*u.hour, 5*u.deg, distance=1*u.kpc) | ||
assert icrs_nokwarg.ra == icrs_2.ra and icrs_nokwarg.dec == icrs_2.dec | ||
|
||
#they also are capable of computing on-sky or 3d separations from each other, | ||
#which will be a direct port of the existing methods: | ||
coo1 = coords.ICRS(ra=0*u.hour, dec=0*u.deg) | ||
coo2 = coords.ICRS(ra=0*u.hour, dec=1*u.deg) | ||
# `separation` is the on-sky separation | ||
assert coo1.separation(coo2).degree == 1.0 | ||
|
||
#while `separation_3d` includes the 3D distance information | ||
coo3 = coords.ICRS(ra=0*u.hour, dec=0*u.deg, distance=1*u.kpc) | ||
coo4 = coords.ICRS(ra=0*u.hour, dec=0*u.deg, distance=2*u.kpc) | ||
assert coo3.separation_3d(coo4).kpc == 1.0 | ||
|
||
#The next example raises an error because `coo1` and `coo2` don't have distances | ||
assert coo1.separation_3d(coo2).kpc == 1.0 # raises ValueError | ||
|
||
|
||
#the frames also know how to give a reasonable-looking string of themselves, | ||
#based on the preferred representation and possibly distance | ||
assert str(icrs_2) == '<ICRS RA=120.000 deg, Dec=5.00000 deg, Distance=1 kpc>' | ||
|
||
|
||
#<-------------------------Transformations-------------------------------------> | ||
#Transformation functionality is the key to the whole scheme: they transform | ||
#low-level classes from one frame to another. | ||
|
||
#If no data (or `None`) is given, the class acts as a specifier of a frame, but | ||
#without any stored data. | ||
J2001 = astropy.time.Time('J2001',scale='utc') | ||
fk5_J2001_frame = coords.FK5(equinox=J2001) | ||
|
||
#if they do not have data, the string instead is the frame specification | ||
assert str(fk5_J2001_frame) == "<FK5 frame: equinox='J2000.000', obstime='B1950.000'>" | ||
|
||
# Note that, although a frame object is immutible and can't have data dded, it | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: |
||
# can be used to create a new object that does have data by giving the | ||
#`realize_frame` method a representation: | ||
srep = coords.SphericalRepresentation(lat=5*u.deg, lon=8*u.hour) | ||
fk5_j2001_with_data = fk5_J2001_frame.realize_frame(srep) | ||
assert fk5_j2001_with_data.data is not None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to admit I didn't quite understand this paragraph. What type is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If that makes sense, how do you think I might change the paragraph to make this clearer? |
||
|
||
#These frames are primarily useful for specifying what a coordinate should be | ||
#transformed *into*, as they are used by the `transform_to` method | ||
# E.g., this snippet precesses the point to the new equinox | ||
newfk5 = fk5.transform_to(fk5_J2001_frame) | ||
assert newfk5.equinox == J2001 | ||
|
||
#classes can also be given to `transform_to`, which then uses the defaults for | ||
#the frame information: | ||
samefk5 = fk5.transform_to(coords.FK5) | ||
#`fk5` was initialized using default `obstime` and `equinox`, so: | ||
assert samefk5.ra == fk5.ra and samefk5.dec == fk5.dec | ||
|
||
|
||
#transforming to a new frame necessarily loses framespec information if that | ||
#information is not applicable to the new frame. This means transforms are not | ||
#always round-trippable: | ||
fk5_2 =coords.FK5(ra=8*u.hour, dec=5*u.deg, equinox=J2001) | ||
ic_trans = fk5_2.transform_to(coords.ICRS) | ||
|
||
#`ic_trans` does not have an `equinox`, so now when we transform back to FK5, | ||
#it's a *different* RA and Dec | ||
fk5_trans = ic_trans.transform_to(coords.FK5) | ||
assert fk5_2.ra == fk5_trans.ra # raises AssertionError | ||
|
||
# But if you explicitly give the right equinox, all is fine | ||
fk5_trans_2 = fk5_2.transform_to(coords.FK5(equinox=J2001)) | ||
assert fk5_2.ra == fk5_trans_2.ra | ||
|
||
#Trying to tansforming a frame with no data is of course an error: | ||
coords.FK5(equinox=J2001).transform_to(coords.ICRS) # raises ValueError | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about being able to create a new XYZFrame from a old ABCFrame? i.e. fk5_2 =coords.FK5(ra=8*u.hour, dec=5*u.deg, equinox=J2001)
ircs2 = coords.ICRS(fk5_2) Like @astrofrog said for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My attitude on this is that "there should be only one obvious way to do it". Adding this way of doing the transform means there's two ways to do |
||
|
||
#To actually define a new transformation, the same scheme as in the | ||
#0.2/0.3 coordinates framework can be re-used - a graph of transform functions | ||
#connecting various coordinate classes together. The main changes are: | ||
# 1) The transform functions now get the frame object they are trasnforming the | ||
# current data into. | ||
# 2) Frames with additional information need to have a way to transform between | ||
# objects of the same class, but with different framespecinfo values | ||
|
||
#An example transform function: | ||
@coords.dynamic_transform_matrix(SomeNewSystem, FK5) | ||
def new_to_fk5(newobj, fk5frame): | ||
ot = newobj.obstime | ||
eq = fk5frame.equinox | ||
# ... build a *cartesian* transform matrix using `eq` that trasnforms from | ||
# the `newobj` frame as observed at `ot` to FK5 an equinox `eq` | ||
return matrix | ||
|
||
#other options for transform functions include one that simply returns the new | ||
#coordinate object, and one that returns a cartesian matrix but does *not* | ||
#require `newobj` or `fk5frame` - this allows more optimization of the transform | ||
|
||
|
||
#<---------------------------"High-level" class--------------------------------> | ||
#The "high-level" class is intended to wrap the lower-level classes in such a | ||
#way that they can be round-tripped, as well as providing a variety of convenience | ||
#functionality. This document is not intended to show *all* of the possible | ||
#high-level functionality, rather how the high-level classes are initialized and | ||
#interact with the low-level classes | ||
|
||
#this creates an object that contains an `ICRS` low-level class, initialized | ||
#identically to the first ICRS example further up. | ||
sc = coords.SkyCoordinate(coords.SphericalRepresentation(lat=5*u.deg, lon=8*u.hour, distance=1*u.kpc), system='icrs') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any plan of how I am thinking about if you implement a new low-level frame how would ping @rhewett There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A registry system like in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that is what I was thinking. How does your registry compare to what we use for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Cadair - the idea is https://github.com/astropy/astropy/blob/master/astropy/io/ascii/core.py#L689 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea I am getting at here is that this would be identical to what we do in the existing coordinate classes. When you define a new class, you give it the decorator You can see examples of how this works by looking in the current I'll add a comment below this in the document indicating that this can mostly be reused from the existing code base. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Cadair @taldcroft - whoops, I collapsed this discussion in the commit I just sent up. If you wanted to say more about this, it might be easier to make a comment against the current diff. |
||
#Other representations and `system` keywords delegate to the appropriate | ||
#low-level class. The already-existing registry for user-defined coordinates | ||
#will be used by `SkyCoordinate` to figure out what various the `system` | ||
#keyword actually means. | ||
|
||
#they can also be initialized using the preferred representation names | ||
sc = coords.SkyCoordinate(ra=8*u.hour, dec=5*u.deg, system='icrs') | ||
sc = coords.SkyCoordinate(l=120*u.deg, b=5*u.deg, system='galactic') | ||
|
||
#High-level classes can also be initialized directly from low-level objects | ||
sc = coords.SkyCoordinate(coords.ICRS(ra=8*u.hour, dec=5*u.deg)) | ||
#The next example raises an error because the high-level class must always | ||
#have position data | ||
sc = coords.SkyCoordinate(coords.FK5(equinox=J2001)) # raises ValueError | ||
|
||
#similarly, the low-level object can always be accessed | ||
assert str(scoords.frame) == '<ICRS RA=120.000 deg, Dec=5.00000 deg>' | ||
|
||
#Should (eventually) support a variety of possible complex string formats | ||
sc = coords.SkyCoordinate('8h00m00s +5d00m00.0s', system='icrs') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recall some discussion that it would also be nice to support
(i.e. no space between the pairs). Is that still the case. I remember it because it will require some interaction between the angle parser and the coordinate parser to find the division in the obvious way. Totally doable, but it means the coordinate parser is no longer as simple as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Are you saying this as a reason why it would be better for all that sort of parsing to live in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more of a code structuring question than an API question, but yes, I think it might be easier if the parsing all lived in a central place, since it has potential to get a little difficult. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. given that there was a clear vote to have this parsing in |
||
#In the next example, the unit is only needed b/c units are ambiguous. In | ||
#general, we *never* accept ambiguity | ||
sc = coords.SkyCoordinate('8:00:00 +5:00:00.0', unit=(u.hour, u.deg), system='icrs') | ||
#The next one would yield length-2 array coordinates, because of the comma | ||
sc = coords.SkyCoordinate(['8h 5d', '2°5\'12.3" 0.3rad'], system='icrs') | ||
#It should also interpret common designation styles as a coordinate | ||
sc = coords.SkyCoordinate('SDSS J123456.89-012345.6', system='icrs') | ||
|
||
#the string representation can be inherited from the low-level class. | ||
assert str(sc) == '<SkyCoordinate (ICRS) RA=120.000 deg, Dec=5.00000 deg>' | ||
#but it should also be possible to provide formats for outputting to strings, | ||
#similar to `Time`. This can be added right away or at a later date. | ||
|
||
#transformation is done the same as for low-level classes, which it delegates to | ||
scfk5_j2001 = scoords.transform_to(coords.FK5(equinox=J2001)) | ||
|
||
#the key difference is that the high-level class remembers frame information | ||
#necessary for round-tripping, unlike the low-level classes: | ||
sc1 = coords.SkyCoordinate(ra=8*u.hour, dec=5*u.deg, equinox=J2001, system='fk5') | ||
sc2 = sc1.transform_to(coords.ICRS) | ||
#The next assertion succeeds, but it doesn't mean anything for ICRS, as ICRS | ||
#isn't defined in terms of an equinox | ||
assert sc2.equinox == J2001 | ||
#But it *is* necessary once we transform to FK5 | ||
sc3 = sc2.transform_to(coords.FK5) | ||
assert sc3.equinox == J2001 | ||
assert sc1.ra == sc3.ra | ||
#Note that this did *not* work in the low-level class example shown above, | ||
#because the ICRS low-level class does not store `equinox`. | ||
|
||
|
||
#`SkyCoordinate` will also include the attribute-style access that is in the | ||
#v0.2/0.3 coordinate objects. This will *not* be in the low-level classes | ||
sc = coords.SkyCoordinate(ra=8*u.hour, dec=5*u.deg, system='icrs') | ||
scgal = scoords.galactic | ||
assert str(scgal) == '<SkyCoordinate (Galactic) l=216.31707 deg, b=17.51990 deg>' | ||
|
||
|
||
#the existing `from_name` and `match_to_catalog_*` methods will be moved to the | ||
#high-level class as convinience functionality. | ||
|
||
m31icrs = SkyCoordinate.from_name('M31', system='icrs') | ||
assert str(m31icrs) == '<SkyCoordinate (ICRS) RA=10.68471 deg, Dec=41.26875 deg>' | ||
|
||
cat1 = SkyCoordinate(ra=[...]*u.hr, dec=[...]*u.deg, distance=[...]*u,kpc) | ||
cat2 = SkyCoordinate(ra=[...]*u.hr, dec=[...]*u.deg, distance=[...]*u,kpc | ||
idx2, sep2d, dist3d = cat1.match_to_catalog_sky(cat2) | ||
idx2, sep2d, dist3d = cat1.match_to_catalog_3d(cat2) | ||
|
||
|
||
#additional convinience functionality for the future should be added as methods | ||
#on `SkyCoordinate`, *not* the low-level classes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor question - do we really want to allow this 'free-form'? It will likely increase the complexity of the initializer, and may impact performance. While this is cool, for a low-level class it's maybe a bit too much flexibility. Why not always use the keyword arguments? (
lat=
is not too many extra characters).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By free-form, you mean allow e.g.
SphericalRepresentation(Latitude(...),Longitude(...)
in addition toSphericalRepresentation(Longitude(...), Latitude(...)
? I could see it either way, now that you mention it, but when writing this I was thinking that these would be regular positional arguments that just check to make sure they're valid (e.g., can't dolat=Longitude(...)
).To clarify, though, I don't think you should necessarily consider the representations to be "low-level" like the frame objects... for frames where the "natural" coordinate system isn't obvious, I think we want the representation objects to be easy enough to use that users can use them if their not sure what they should do otherwise. That is, it should be ok to expect users to be interacting with representation objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and this is something that we could push off to the implementation stage by trying to do it and seeing if it really does impact performance that much - if so, just do the simple positional args/kwargs, and if not, do the more free-form version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I meant that I don't think we want to allow:
in addition to:
so that there should be only one 'order'. So I'm specifically disagreeing with the
order doesn't matter
statement. These can be keyword arguments which if specified can be passed in any order, but if passed without keyword arguments then the order should match (i..e no magic with swapping lon and lat). Does this make sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, as mentioned below, we could start off being more strict and then relaxing the rules (better that way around than the other way).