Allow null SkyCoord transform from NoFrame to NoFrame - #2543

Don't forget milestone & labels :)

I'm actually 👎 to this. That is, I see the current behavior as a feature, not a bug (although I'm open to being convinced).

My reasoning is that this (along with other places like match_coord), you really should be explicit about what frame you're in. That was part of the original design of coordinates and implicit in APE5. If we allow this, than users will likely just use NoFrame most of the time without ever thinking about the frame they're in. And that leads to lots of subtle errors in science settings. I don't think it's that big of a deal to require them to at least do frame='icrs' if it makes them ask themselves what frame they really want.

Why should you not be able to get the separation on the sky in a NoFrame? Requiring a frame always in this case leads to the also-undesirable situation of people just inserting an arbitrary frame into data in order to get the function to do something. I.e. imagine you are given a catalog of object coordinates and but not the frame and you want to find nearby pairs. To get this to work the user needs to invent a frame when that information is not actually relevant to the problem. Then let's say they pickle the coordinates object and hand it off to their collaborator, who unwittingly assumes that the specified frame is actually correct.

My other argument is that this software is primarily aimed at research astronomers and you can assume some basic competency about astronomy. I think that the person who will be entirely unaware of coordinate frames will be the same one who will just blindly add frame='icrs' to make it work.

Hmm, I see your point about users "inventing" frames @taldcroft ... But my point isn't so much that we can ever hope to catch things like that, but rather that this way we force the user to at least think about it a little, even if long enough to decide "eh, it doesn't matter". I'm thinking with that, more people will say "Well, it is in fact ICRS, I was just too lazy to type that... but now I will."

I think it might be useful to get another opinion on this, though - perhaps @astrofrog?

I re-milestoned this for 0.4.1. Perhaps it's best to hash this out after the 0.4 release and decide if it's a bug or a feature.

Fair point - definitely easier to turn this on in the future than go the other way!

It's a feature!

I was about to file a feature request asking for this to work:

SkyCoord(0, 0, unit='deg').separation(SkyCoord(10, 20, unit='deg'))

I'm using this in a simple generic spherical geometry calculation.

@cdeil - in the previous discussion @eteq was saying that the fact that your use-case fails is a "feature", and I call it a bug. So I think you are with me on the "failing here is a bug" side.

There's no spherical geometry code in Astropy besides astropy.coordinates.
I think using SkyCoord without frame for spherical geometry calculations is nice, no?
Is this briefly explained in the docs?

@cdeil - I guess I see your point here... There's not necessarily another way to do this easily, and sometimes you just want it done.

The problem here is that there's an essential tension here between "we want users to actually think about what they mean" and "we want to make things as easy and fast to do as possible". One of our goals (or perhaps it's mainly my goal) is to prevent the sorts of mistakes that come from people playing fast-and-loose with these sorts of definitions. time, for example, does a good job of this, and I think it will prevent a variety of future subtle mistakes. But it may be that I'm being overly-cautious here. @astrofrog or @adrn or @Cadair, do you have thoughts on this?

Another option to consider, and that I'd be willing to put in for 0.4 if @cdeil and @taldcroft are ok with it: instead of raising an exception, have this (and similar usage in separation and match_to_catalog_*) generate a warning with a message along the lines of 'you are trying to do something that might be wrong if you're doing it between two different frames'. That would satisfy me that people would be forced to at least think about it for a moment, while still enabling the sort of usage you're asking for.

@eteq I'm 👎 on generating a warning ... then I'll have to suppress it in my code, which is more complicated than adding a dummy frame. Maybe a frame="sphere" that's not connected to any other system if the danger of allowing frame=None is really that high?

@cdeil - oh, maybe I'm misunderstanding the complaint: are you saying that you'd be fine with something like:

SkyCoord(0*u.deg, 0*u.deg, frame='sphere').separation(SkyCoord(10*u.deg, 20*u.deg,  frame='sphere'))

?

I'm perfectly fine with an explicit "generic" frame. My concern is about situations like this:

ra, dec = ... load from a file in ICRS/J2000 ...
c1 = SkyCoord(ra, dec)  # you've gotten used to ignoring the frame keyword...

... a bunch of stuff in between ...

ra, dec = ... load from another file, in FK4 ...
c2 = SkyCoord(ra, dec)

c1. separation(c2)  # WRONG ANSWER!!

I'd prefer the default NoFrame to be that "generic frame" instead of creating a separate 'sphere' frame ... wouldn't having both be more confusing for the user? (which to use to store generic spherical coordinates?)

But if you don't want that, then yes, writing frame='sphere' in my code where I simply want to do spherical geometry calculations would be better (more explicit, and maybe even less memory / faster?) than having to use a dummy frame='icrs'.

@eteq - I've already weighed in but I just want to reiterate the general Python dogma of treating users as adults and even more, research astronomers who understand at a solid level the idea of coordinate frames . There are a thousand ways to get the wrong answer with astropy. Some one can fit an unreasonable model to data but we don't purposely make it difficult to do the simplest thing like fit a line. But that is just my view.

research astronomers who understand at a solid level the idea of coordinate frames

That's my point: I know a lot of research astronomers who do not understand the many subtleties here (I have done multiple surveys, both of the "walking down the hall" variety and the "ask people at conferences" sort).

I also disagree that this is "general Python dogma" - after all, the zen (which is about as dogmatic as it gets) clearly says "Explicit is better than implicit" and "Errors should never pass silently". Exactly how much that applies here is subjective, but I see those as telling us not to do this. But of course it's also true that "Practicality beats purity", and that's the issue at hand.

All that said, I could accept caving on this, as I would in fact use it myself in a lot of science code... But I would like to hear at least a couple more viewpoints from other people who have considered the arguments here.

@astrofrog @mhvk @adrn @Cadair - waiting for your 👍 or 👎 votes on whether to accept this PR to allow computing the separation or matching between two or more NoFrame coordinates (where no frame was explicitly specified). It's all ready to go, we just have a stalemate now with the current discussion participants.

I put this back to 0.4 for the moment because it is still in consideration for inclusion.

Well, this is a controversial feature for sure :) My vote is to allow this, but (as @eteq suggested) emitting a warning pointing out that the coordinates are assumed to be in the same frame. This seems like a compromise. Note that as for all warnings, this will only appear the first time, so it's not like it will clutter up users' terminal.

We could always take another vote later for whether or not to keep the warning for 1.0, once people are using the new astropy.coordinates.

I'm really 👎 on warnings like this for the reason @cdeil mentioned, because suppressing warnings is much more pain and confusing than supply a dummy frame name. (I don't know off the top of my head how to suppress a particular warning for a particular line of code, is there some context manager to do this? Probably, but using astropy in an "endorsed" manner shouldn't require this sophistication).

Hmm, this is a tough one. Overall, I'm 👎 as I think this is correct behaviour, though I do see that we want to allow the use cases mentioned. I don't really like the warning route to "half" allow it: things are either OK or they are not.

The reason I think it should generally not be allowed comes in part from comparing to other places in astropy:

• In Quantity, it would not be possible to subtract two quantities with different unknown unit; this suggests the current behaviour is correct; it would, of course, be possible to subtract .value -- maybe similarly one can get separation from viewing the coordinates as a pure representation object.
• In Time, it used to be the case one could not even initialize without a scale, but now the default is utc; this suggests not allowing NoFrame at all or making icrs default.

I see several options:

1. The representation classes gain their own separation methods, so that it becomes possible to do @cdeil's example using (Unit)SphericalRepresentation. [I think they should have this method regardless...]
2. We allow it only if the angles to be compared are from the same object (but how to keep track...).
3. We add an ignore_frame option (or so) to separation, which suppresses the exception (or warning, if people do not share my dislike).

p.s. I don't think this should hold up 0.4.

I think I'm tilting towards 👍 to the warning idea, particularly with @astrofrog's

We could always take another vote later for whether or not to keep the warning for 1.0, once people are using the new astropy.coordinates.

Having recently been using some of this stuff for science work, I've noticed myself sometimes using 'icrs' as a crutch in situations where I want SkyCoord convenience functionality, but where the frame is unknown/unimportant (e.g., simulated coordinates, or photometry where the absolute astrometry is not well-known). So that has convinced me that there's a role for something allowing this, as @cdeil suggested.

But I still have my initial concern that if we silently default to this, users will mis-use it, because they will try to use it like software they've used before, which generally is not nearly as advanced as this in terms of explicit frames. So that's why I like the warning: it forces them to think about it because they see the message once, but after that they can ignore it. We do this in other parts of Astropy, and it's exactly what warnings were designed for in python. Telling people "this will run, but it may not behave the way you think it will behave".

• Here's another possible twist on the warning approach: have the warning say something like "You've tried to combine two SkyCoords without explicitly choosing a frame. This may be a mistake if they are actually in different frames. Use the frame keyword to specify the actual frame. If you meant to do this, and think this warning shouldn't exist, go to https://github.com/astropy/astropy/issues/#### and give us your feedback there." If we get a lot of people saying "I hate this warning, remove it", we can take it away in 1.0.

Oh, and re: @mhvk's options:

1. The representation classes gain their own separation methods, so that it becomes possible to do @cdeil's example using (Unit)SphericalRepresentation. [I think they should have this method regardless...]

I don't think we should do this, because the problem described here applies for all convinience methods on SkyCoord, including e.g. match_to_catalog_*. Those are sometimes useful in the sort of "no frame" setting @cdeil described

2 . We allow it only if the angles to be compared are from the same object (but how to keep track...).

Just as you say, I don't think there's a way to reasonably do this because the idea of "same object" is ambiguous

3 . We add an ignore_frame option (or so) to separation, which suppresses the exception (or warning, if people do not share my dislike).

This could work, but I prefer a slightly different version of this that occurred to me right now - lets call this "4.": in #2629 we discussed (and I think agreed on) the idea of a generic Equatorial frame which doesn't transform to-from anything (other than itself). We could make a trivial subclass of that called something like DefaultEquatorial, and have that be a special frame that prevents the use of separation et al., as per the current master. But if the user explicitly asks for Equatorial as a frame, they get a "generic" frame that is only different from the current default in that it allows separation* and the other convinience methods to work with other Equatorial frames. So then users have to explicitly ask for a frame that operates the way @cdeil wants, so they at least have to consider the idea of frames (which mostly satisfies me).

I'm 👎 on the warning idea.

My current use case for this is a PSF class where I want to do sky distance calculations to evaluate a PSF model in any sky frame in affiliated package library code, not an end-user script.

Either simply allowing this as @taldcroft and I want or any of the methods proposed above to explicitly declare "I want to use SkyCoord simply for spherical geometry calculations" are better than me having to litter my code with this to prevent my users from seeing an incorrect warning message:

from astropy.tests.helper import catch_warnings
from astropy.coordinates import PotentiallyIllegalFrameWarning
with catch_warnings(PotentiallyIllegalFrameWarning):
    offset = SkyCoord(0, 0, unit='radian').separation(SkyCoord(lon, lat, unit='radian'))
    return psf.eval(offset)

IMO raising warnings for correct and common usage patterns is a bit silly and extra arguments to declare explicitly what I want to do (if really needed for "end-user safety") are better.

But I'm starting to repeat myself, so I'll shut up now and if this doesn't go in Astropy 0.4 I'll just use 'icrs' as a dummy frame with a comment in my code, which is OK.

Intuitively, I don't think SkyCoord should be used just for the purpose of doing spherical geometry - its purpose is to point to actual locations on the sky, not just locations on a sphere, right? Is there not an angular separation routine you can use directly?

Separations are the use-case I'm thinking of, and it's quite common to have catalog data with coordinates where the exact frame really doesn't matter. You just want to know how many close pairs there are in this catalog. Or you've measured the positions of a bunch of Chandra X-ray jet knots using DS9, typed those into the computer and now want to know the separations.

As I argued before, I think it is actually worse to force people into the habit of assigning a dummy frame so they can get on with their research, because then you've trained people to invent information to get around this issue. @eteq hopes that people will do the right thing and dig through the paper or Chandra documentation to find the actual reference frame, but I think that most astronomers in this situation will just put in 'icrs' and move on.

I think @astrofrog is right that the use cases mentioned all involve using SkyCoord as a shortcut to do spherical geometry, which does not feel right (just as it is not quite right for Time). Of course, internally, we use coordinates.angle_utilities.angular_separation and maybe the easiest way out is to make that available under coordinates itself, and have it referenced in the exception message.

Though this does bring me back to my option (1), that the representation classes should be able to calculate separations. In that case, one could use these directly (appropriate in @cdeil's example and @taldcroft's Chandra one) or, if one were still to use SkyCoord (appropriate for the catalog example), one could do:

coord1.spherical.separation(coord2.spherical)

Which basically means that you let the user decide whether s/he is confident the coordinates are in the same system, perhaps not too differently from the user being able to do q1.value - q2.value.

A small advantage of this approach is that it would also simplify the implementation of SkyCoord.separation itself: it now only has to take care of the transformation between frames, and then does exactly the above.

We have two cars, and one is a Japanese car that is designed with simplicity in mind, and the other is from a German company known for performance. The Japanese car has a button on the door that you can push to lock the doors. This is a common operation, it works great, and I use it all the time. The Germans instead were fearful that users would accidentally lock themselves out of the car, so they made it actually impossible to lock the car without getting your keys out of your pocket.

My plea is to be less fearful for users, less pedantic, and just let everyone do the simple and obvious thing. The suggestions so far (warnings or coercing into a special representation) all require googling or carefully reading the docs to use effectively. I just want software that is basically obvious for the simple things, even if it means trusting users to not lock themselves outside the car.

Hmm, perhaps there it should be possible to use frame=AnyFrame? One for which it is assumed that the frame is the same as any other? At least, that would make the meaning clear, and keep the option of using NoFrame if one wants to be strict. It has some precedent, in being similar to how we treat TimeDelta(..., scale=None) (which is assumed to have the scale of what Time it is added/subtracted from, similar to a quantity with time units). I'd personally prefer this not to be the default, but do not feel strongly about that.

Hmm, perhaps there it should be possible to use frame=AnyFrame? One for which it is assumed that the frame is the same as any other?

That seems to be going way beyond the original idea, which can be restated as "Every astropy frame shall have an identity transform to itself". I think this is a much more limited concept than a frame that has an identity transform to every other frame. When we implement proper coordinate equalities this could lead to nice transitivity paradoxes with a = b = c but a != c.

Since it seems there are strong opinions on both sides, and just to see if this would be an acceptable solution, what if we assume instead that if no frame is given in SkyCoord, it defaults to ICRS? Why would we do this?:

• I think it is what FITS assumes by default for an RA/Dec system if no equinox is present in the header (see http://fits.gsfc.nasa.gov/standard30/htmlfiles/fits_standard3000089.html)
• I think Kapteyn also assumes this for their coordinate package if no frame information is given (see 'Notes' in http://www.astro.rug.nl/software/kapteyn/celestial.html#celestial.skyparser)
• In the Time class if a scale is not specified, we assume utc (rather than assuming a blank scale) so that things just work, so by doing this, we are consistent with Time.
• It is, I think, the only scale which does not require any additional information (epoch, equinox, etc.) in order to be correctly specified, so there is no other logical choice for a default.

The advantage is, if the user really doesn't care, things will work, and if they care, they will notice in the __repr__ what is being assumed, and it may prompt them to change the frame if they realize it's important. This would solve this whole issue since SkyCoord would never be frameless.

No one opposed making scale='utc' the default for Time (https://groups.google.com/forum/#!topic/astropy-dev/I_7-HSpzr94), so I don't see how this would be any different.

👍 to @astrofrog's suggestion to put a default frame='icrs'.
(Take this is a user's opinion, I'm haven't looked at the SkyCoord implementation).
If we do this, is there still a need for NoFrame at all or would it be removed completely?

I'm fine with an icrs default as well, and like the similarity to what we do for Time.

Do still feel the representations should be able to calculate separations, but that's for a different PR.

👍 to @astrofrog's suggestion. I believe NoFrame would get removed then.

@eteq - I think we just need your opinion about defaulting to 'icrs' - does this make sense to you? If so, do you want to update this PR to reflect this, or should we open a new PR?

@astrofrog et al. - I don't like this too much for the reasons I gave above about users mis-using this frame... But @taldcroft has a fair point that users will probably just do this anyway, which is worse because then it seems like it should be ICRS. And anyway, it's clear I'm in the minority, so I'm willing to just accept that defaulting to 'icrs' is the least of all evils.

(@taldcroft - apparently I like German-style cars? Oddly, my car is a Toyota... I'll have to look at Volkswagen next time 😉)

Oh, and it might make sense for me to just add this as part of finalizing #2629 , as it might conflict. But it might not, so if @taldcroft gets to doing this before, it would probably also work to modify this PR.

@eteq - I haven't done this yet. I think it makes sense to roll into #2629 if you don't mind. I will close this PR in any case since the original idea has been overtaken by events.

@eteq - btw, if you want me to work on this (from current master I guess), I can. Even though the actual code changes will be pretty trivial, I suspect this change will have a moderately large footprint in tests and docs. Nothing tricky, just grinding.

@taldcroft - I have now this implemented in 8ed4d01 of #2629