These are chat archives for astropy/astropy

7th
May 2014
Thomas Robitaille
@astrofrog
May 07 2014 19:37
@eteq - just to let you know, I'm around if you want to discuss my latest suggestion (https://github.com/astropy/astropy/pull/2422#issuecomment-42472998) further
Erik Tollerud
@eteq
May 07 2014 21:06
Still there, @astrofrog ?
Thomas Robitaille
@astrofrog
May 07 2014 21:21
Yes, I'm here for another 10 minutes
@eteq - sorry for not replying sooner
Erik Tollerud
@eteq
May 07 2014 21:21
Ok, will try to make this quick, then - np

So you said:

the fundamental issue here is that users should now no longer instantiate frames with data (correct?) and should use SkyCoord instead

That’s not quite true
It’s sort of “use SkyCoord unless you have a particular reason to want more control, in which case use the low-level objects"
So the low-level frames do have some conviniences and are meant to be user-facing
Thomas Robitaille
@astrofrog
May 07 2014 21:23
I see - but if it's an 'advanced' feature it might still be ok to have it in a special class method, to keep the API simple for the 99%, right?
can you give examples of when the SkyCoord would not be appropriate, just so I get a better understanding of the use cases?
Erik Tollerud
@eteq
May 07 2014 21:24
It depends on how many people need user-specified coordinates. We know the answer is “non-ignorable”, but it may be, as you say <1%… or it may be a lot more now that it’s relatively easy
Here’s an example
Say I want to do a transformation from ICRS->Galactic, but I want to go through FK4 instead of FK5 (FK5 is our default because it avoids the E-terms)
then I would do something like:
ci = ICRS(ra=foo, dec=bar)
cfk4 = ci.transform_to(FK4(obstime='J1983’)
cfk4.transform_to(Galactic)
Thomas Robitaille
@astrofrog
May 07 2014 21:27
Can't you then do:
ci = SkyCoords(...)
cfk4 = ci.transform_to(FK4(obstime='J1983’)
cfk4.transform_to(Galactic)
Erik Tollerud
@eteq
May 07 2014 21:27
I guess you could do that with a SkyCoord, too, but it’s subtly different
well, not for FK4… so maybe a bad example :wink:
Lets instead say @adrn’s SgrCoordinates
those you would have to implement as a new frame, and if you further wanted to have a method along the lines of SgrCoordinates.distance_from_sgr_dwarf(), you couldn’t do that with SkyCoord
(Also, a side point re:your option 7… I think that would be a lot of work to re-implement, because some of the transforms assume you can use a frame to generate a frame with data)
(so it might mean we couldn’t include this in 0.4)
Thomas Robitaille
@astrofrog
May 07 2014 21:32
I see what you mean re: SgrCoordinates - however, just to check, if they are meant to be used directly, then how would you instantiate the with angles that are unit-ambiguous, e.g. 13:14:15 ?
Erik Tollerud
@eteq
May 07 2014 21:32
(Oh, also, what you’re suggesting in option 7 is partly already there in realize_frame)
like this:
(whoops, keep forgetting the dual use of the enter key here…)
SgrCoordinates(Angle(’13:14:15’, u.degree),Angle(‘12d43m98s’))
Thomas Robitaille
@astrofrog
May 07 2014 21:35
right - I guess I just think that users are going to not understand why in this specific case, one can't specify unit= when SkyCoord supports it and the frames used to
Erik Tollerud
@eteq
May 07 2014 21:35
The idea in my head is that you have to understand the internals well enough to write your own frame that it’s ok to expect those people to use the full OO framework. SkyCoord is there for those who want to do it faster or are afraid of OO
I see your point on that, but I think there’s no real solution: adding unit makes writing a frame class initializer a lot harder, which was one of the main reasons for APE5. So we have to get rid of it at some point
Thomas Robitaille
@astrofrog
May 07 2014 21:37
Just to be clear, I'd actually be ok with leaving everything pretty much as-is, and forgetting all my options, but I don't understand why we can't simply allow unit= on frames
ah sorry wrote too soon
hmm
Erik Tollerud
@eteq
May 07 2014 21:38
hah, np - I guess my attitude is that because we know we’ll have to break it eventually, better to do it all at the same time
but I’m not hugely attached to that - if we think gradual is better, it wouldn’t be that hard to do
Thomas Robitaille
@astrofrog
May 07 2014 21:38
does it really add a fair bit of complexity to allow unit in the frames?
my motivation is this:
if @adrn writes SgrCoordinates and sends it to people who aren't as tech-saavy, then why should they have to do SgrCoordinates(Angle(’13:14:15’, u.degree),Angle(‘12d43m98s’))?
Erik Tollerud
@eteq
May 07 2014 21:40
oh, they don’t
Thomas Robitaille
@astrofrog
May 07 2014 21:40
but what would they do?
Erik Tollerud
@eteq
May 07 2014 21:40
(typing an example now, one sec)
Thomas Robitaille
@astrofrog
May 07 2014 21:40
np :)
Erik Tollerud
@eteq
May 07 2014 21:41
c = SkyCoord(’13:14:15’, ‘12d43m98s’, unit=(u.degree,u.degree), system=’sgrcoordinates’)
c.distance_from_sgr_dwarf()
But for @adrn to implement that, he has to have access to data in the SgrCoordinates class
Thomas Robitaille
@astrofrog
May 07 2014 21:43
Oh but then we are back to my point that users never need to instantiate frames with data directly, so in principle one could remove the data from the initializers and always use realize_frame or another similar context manager?
Again, to be clear, data in the frames is fine by me, but just trying to simplify the __init__ for users
Erik Tollerud
@eteq
May 07 2014 21:44
I guess that is possible, now that we’ve gone through it, but I guess I don’t see why that’s any better than doing it in the initializer.
Wait, are we talking about the compatibility classes, or the actual “for the future” frames - I assume the latter, right?
Thomas Robitaille
@astrofrog
May 07 2014 21:45
This is the real classes, not any kind of compatibility classes
My reasons are that it complicates the docstring for users, and also that if we keep them at the top-level currently then they will give a false sense of backward-compatibility in some cases and not others
Erik Tollerud
@eteq
May 07 2014 21:46
hmm… ok, I think you’re convincing me … give me a sec to collect my thoughts
Thomas Robitaille
@astrofrog
May 07 2014 21:46
no problem :) sorry for bombarding you with opinions, I'm not actually sure what my favorite option is
but just that I'm worried the status quo will confuse some users
Erik Tollerud
@eteq
May 07 2014 21:46
yeah, I totally understand. I’d rather we decide now than change it again in the next version
Thomas Robitaille
@astrofrog
May 07 2014 21:47
Moving the frames out of the top-level doesn't seem ideal since it requires an additional import statement
Anyway, will let you think about it :)
Still here for another 5-10 min
Erik Tollerud
@eteq
May 07 2014 21:48
k

Here’s where I am right now, I think: Users need to see the frames to write code that makes good use of the frame specification, E.g.,

fr =FK5(equinox=‘J1987’)
...
skycoord.transform_to(fr)

So that calls for them being top-level, if possible. But implementing a class method the way you described is problematic right now, because I think it will require a lot of work to change this, and it is a major departure from APE5.

So the question is how (if?) we want to catch the case of users doing c = ICRS('13d13m13s', '14h14m14s’)

Actually, now that I think about it, why is that bad? They’ll get something that is still what they want, just not our “recommended”/“easiest” interface
Erik Tollerud
@eteq
May 07 2014 21:53
If all of the documentation and tutorials use SkyCoord, presumably those who don’t know any better will learn it that way?
wait, @astrofrog : yet another option occurred to me: your option 3 + my option 5: keep low-level frames at top-level, but if units is present, it yields a SkyCoord and shows a deprecation warning.
Adrian Price-Whelan
@adrn
May 07 2014 21:56
FYI: i'm reading along but my head is spinning
Erik Tollerud
@eteq
May 07 2014 21:56
Hah, fair enough
Thomas Robitaille
@astrofrog
May 07 2014 21:56
Ok, so if we keep what you have at the moment, some (but not all) users who have been using it before will be confused that c = ICRS('13d13m13s', '14h14m14s’) works but c = ICRS('13:13:13', '14:14:14’, unit=...) no longer does, and will likely report it as a bug. But if you're ok with dealing with those users than maybe we can keep it as-is. In the end, the 'ideal' solution IMHO is re-implementing the unitoption.
Erik Tollerud
@eteq
May 07 2014 21:56
re-implementing unit permanently, or as a temporary measure?
(I’m tilting in the direction of that, but as a temporary measure)
Thomas Robitaille
@astrofrog
May 07 2014 21:57
Personally, i think it should be permanently (otherwise frames are in a weird state of being able to deal with some string input but not all)
Adrian Price-Whelan
@adrn
May 07 2014 21:57
FWIW, I'm leaning towards agreeing with @astrofrog on the unit kwarg... it doesn't add that much complexity to the initializer, does it?
Erik Tollerud
@eteq
May 07 2014 21:57
It does!
Thomas Robitaille
@astrofrog
May 07 2014 21:57
So what about...
my option 1
simply disallow string input to frames to avoid the inconsistency
Adrian Price-Whelan
@adrn
May 07 2014 21:58
Wait, sorry, is it easy to explain why? I can look at the code again if not :)
Thomas Robitaille
@astrofrog
May 07 2014 21:58
but allow it temporarily (along with unit) and redirect to SkyCoord
Erik Tollerud
@eteq
May 07 2014 21:58
hmm… I’m liking this
Thomas Robitaille
@astrofrog
May 07 2014 21:58
If frames are low-level and for devs, string input seems unecessary
So allow only quantities/angles/lon/lat but no string
allow string + unit temporarily and deprecate in 0.5
Erik Tollerud
@eteq
May 07 2014 21:59
But it might break the SkyCoord implementation of __init__, which leans heavily on the low-level frames for initialization
Adrian Price-Whelan
@adrn
May 07 2014 21:59
I actually like what @astrofrog is suggesting but yea it will require modifying the initializer for SkyCoord
Erik Tollerud
@eteq
May 07 2014 22:00
Can one of you look into whether you think that will be really hard or fairly easy?
Thomas Robitaille
@astrofrog
May 07 2014 22:00
maybe it's the lesser of many evil solutions though? ;)
Erik Tollerud
@eteq
May 07 2014 22:00
if the latter, I say go for it
yeah
if it’s not going to require a week of debugging, I :+1: for it
(just to be clear, I think this is option 9 :wink:)
Adrian Price-Whelan
@adrn
May 07 2014 22:01
haha
I could look in to it, but not until later tonight because 4 grads just defended and I'm being dragged out to drink beer (such a hard life)
Erik Tollerud
@eteq
May 07 2014 22:01
1st world problems...
Adrian Price-Whelan
@adrn
May 07 2014 22:01
gotta run but will catch up on discussion when i get back
Erik Tollerud
@eteq
May 07 2014 22:02
Ok, or @astrofrog , would you rather? (to familiarize yourself with the code) I’m fine either way, but I already have a fair amount of stuff to do there
Thomas Robitaille
@astrofrog
May 07 2014 22:02
I'm off to bed soon, but I can try and look into it a bit tomorrow, but would be helpful if @adrn and yourself could also look a bit
let's plan to touch base later tomorrow
I'll report back what I find
Erik Tollerud
@eteq
May 07 2014 22:03
ok, that seems reasonable. I’ll try to get the other stuff in today (incl. rebase), and then we can re-evaluate tomorrow
ok, I’ll leave a note on the issue summarizing the discussion
Thomas Robitaille
@astrofrog
May 07 2014 22:04
Great! Then plan B for 0.4 is to just implement the unit -> SkyCoord?
Erik Tollerud
@eteq
May 07 2014 22:05
yeah, that sounds good
that’s trivial
Thomas Robitaille
@astrofrog
May 07 2014 22:05
Ok, then I'm happy with that
we have a PLAN!
Erik Tollerud
@eteq
May 07 2014 22:05
yay plan!
Thomas Robitaille
@astrofrog
May 07 2014 22:17
hmm, I think SkyCoord may already do the string parsing
still need to look closer
Erik Tollerud
@eteq
May 07 2014 22:17
well that suggests this will be very easy!
Thomas Robitaille
@astrofrog
May 07 2014 22:17
but need to double check though
Erik Tollerud
@eteq
May 07 2014 22:17
k
Thomas Robitaille
@astrofrog
May 07 2014 22:18
Would it make sense to disallow string input for representations too in that case? (I say yes)
brb
Erik Tollerud
@eteq
May 07 2014 22:18
that does seem to be self-consistent…
Another thing I noticed just now: the SkyCoord.frame, SkyCoord.frame_cls, and SkyCoord.coordobj properties are confusingly named. That should be cleaned up.
Thomas Robitaille
@astrofrog
May 07 2014 22:20
the reason I ask about representations is because frames don't actually interpret the strings, right? they just pass it on to representations
so I disabled string input on representations and SkyCoord didn't care
Erik Tollerud
@eteq
May 07 2014 22:21
ok, cool, then I think this is an easy fix
Thomas Robitaille
@astrofrog
May 07 2014 22:21
do you want a PR to modify the representation part?
Erik Tollerud
@eteq
May 07 2014 22:22
yeah, that be awesome
do you think you’ll do it soonish (before tomorrow)? Just so I know when I should do the rebase
actually, @astrofrog, igg now anyway - if you think it’s soonish, just issue the PR, otherwise leave a note on the issue if you don’t want me to rebase
Thomas Robitaille
@astrofrog
May 07 2014 23:32
@eteq - don't worry about me in terms of rebasing - you can rebase if you have time, or not, but my changes should be easy to rebase on your branch