Change argument order in AngularSeparation from (lat, lon) to (lon, lat) - #902

Good catch - I missed that when I submitted a pull request before to change the argument order. I'm not sure what the best way is to proceed. I don't think this is a class users are going to be initializing directly, so maybe we can have a warning that gets emitted only if initialized directly?

@eteq @iguananaut - do you have any thoughts?

As @astrofrog said, I doubt anyone is actually initializing from this directly, aside from the code actually in astropy. And I definitely agree they should match the convention elsewhere.

However, it is technically part of the "public" API, so if we are actually going to be consistent about deprecating we should put up a warning for 0.3 and then actually do it in 0.4. I think that might be counter-productive, though, if no one is using it yet, because they might start in 0.2.

We could switch it for 0.3, but put up a warning that says something like "FYI, these have been swapped, if you were actually using this code in 0.2". We would want to silence this where it's used in the astropy core, of course.

@eteq I'm not sure why you say that probably no-one is using this directly. I frequently compute the angular separation between two points on the sphere in my scripts. Should I be using some other class / function?

Like @eteq said, I also think keeping the wrong order for the 0.3 release is bad. Attached find a commit that simply fixes the order. Acceptable?

Should I add an entry to some document where we accumulate deprecations and API changes for the 0.3 release notes?

@cdeil - the intent when writing this was for it to be used as it is shown in http://docs.astropy.org/en/v0.2.1/coordinates/separations.html - that is, use coordinate objects, and get the separation between the objects, instead of explicitly creating an AngularSeparation object. Is there some use case where this is not sufficient?

Regardless, though, it was made public partly because we probably can't forsee all use cases, so people should have the option of using it. So it's probably true we should make it consistent.

@cdeil - we definitely need to have some sort of deprecation warning pop up when this happens. I'll issue a PR against your branch to do what I'm thinking here.

You're also right that we may want to have somewhere where we note API changes. @iguananaut @astrofrog - do you think we should add this to the "what's new" page in a section called "API incompatibilities" or something like that?

@eteq - yes to your question about adding this to the what's new page.

@eteq Yes. 100x yes.

Alright, I added a PR in cdeil#2 that implements this warning and adds a doc section about version-incompatible changes, @iguananaut, you might want to check that out either there or after it gets merged here.

Ready to merge?

Why didn't travis-ci test the 407eeb4 or 02b24d3 commit?
All that I can see is that eb46cc8 was tested successfully by travis-ci.
Is this an issue we should report to github and / or travis-ci?

It looks like this needs to be rebased?

Thanks for the tip. Rebased. travis-ci scheduled a check of the right commit: 0e4318a
https://travis-ci.org/astropy/astropy/builds/6274928

The travis-ci results were weird ... half of the configs worked and others failed in different ways. See e.g.:
https://travis-ci.org/astropy/astropy/jobs/6274935#L1115

I rebased and force pushed again. Let's see...

@cdeil - FYI, the reason why some of them succeeded is because the egg_info builds do not run the tests - they just make sure astropy's setup.py is syntactically correct (basically). The build_sphinx one, meanwhile, succeeds even if the tests fail, because it builds the docs instead of running any tests.

Anyway, I think the item @astrofrog pointed out is the problem. That's my fault, but it's probably easier if you just fix it seeing as how it's your branch here.

@eteq In ae5513f you made a change that meant that in the case

 AngularSeparation(lon1=0, lat1=45, lon2=0, lat2=0, units='deg')

lat1 wasn't converted from deg to radians as it should be.

6415688 fixes this case and covers it with a unit test.

What is the motivation for this special case anyways?

        if 0 == lon1 == lat2 == lon2:
            sepval = lat1

I think it's not needed and we should remove it to simplify the code a bit?

@cdeil - that's actually meant for something different - the idea is that we want to allow AngularSeparation(somenumber,unit='deg') and have it work to give a number - that was broken by this PR because . But you're right the change I made in ae5513f was not implementing that correctly - I'll issue a PR against your branch shortly (and add a test for this case).

@eteq - maybe for that it should be using None instead of 0 for the unset parameters, to clarify?

@astrofrog - we could do that, but it's actually still completely correct as-is, because having lat1, lon2, and lat2 all 0 means that the value for lon1 will always be the exact same as the distance between those points. So that allows a shortcut for this special case, which is worthwhile because the vicenty distance is a relatively expensive numerical operation. I think this gets used internally in some places.

Now that you mention it, though, None could also be allowed. I'll adjust my PR to reflect that.

I think this was originally intended to allow usage like AngularSeparation(1.2,units='deg') instead of AngularSeparation(1.2,0,0,0,units='deg'), but the hang-up is that unit then has to have a default. Once this is merged, I'll issue a separate PR to try to deal with that.

I think this is ready?

@eteq Concerning the issue of howto initialize AngularSeparation. Maybe the best solution is to turn AngularSeparation into a function angular_separation that simply returns an Angle object. Then if a user wants to create an angle representing an angular separation she can do

d = Angle(42, units='deg')
d = angular_separation(42, 43, 44, 45, units='deg')

I don't see the need for an AngularSeparation class (and the associated complication of how to initialize it either from an angle, or from (coordinate1, coordinate2) or from (lon1, lat1, lon2, lat2).

If you like this idea we can simply close this PR, because this change / deprecation process for AngularSeparation becomes unnecessary.

@cdeil - the reason for AngularSeparation as a separate class is twofold:

1. To allow the arcsecs and arcmins properties. However, if Unit/conversions as properties #837 gets merged and we change Angle to be a Quantity subclass (which we're currently planning on doing), that's a moot point because Angle will get arcsec and arcmin anyway.
2. Overridding the +-*/ operators. Angle allows addition and subtraction with other angles and multiplying by scalars, while AngularSeparation allows no such operators. I think the motivation for this is that it doesn't make sense to operate on a 1D angle that representes a distance between two other 2D coordinates. But thinking back on it now, I can't really justify that very strongly even to myself.

I propose we merge this, and then revisit removing AngularSeparation after #837 is in and Angle has become a Quantity subclass. At that time I think you may be right that AngularSeparation becomes better just as a function. Does that sound good, @cdeil ?

@eteq As you like ... merge or simply close as irrelevant.

Concerning the question whether to remove the AngularSeparation class: I think there are cases where you want to do separation_AB + separation_BC, e.g. when you want to compute how far you have to move your telescope to point to A, then B, then C ... so making AngularSeparation a special Angle that doesn't allow + like normal Angles will just cause confusion and boilerplate code, I think.

Well, that's exactly the point of the distinction between separating AngularSeparation and Angle. An Angle is in principal a 2d construct (angles in a plane), so it's fine to add or subtract it from other angles because there's only one direction to go. But AngularSeparation is always an angle in three dimensions (that is, an on-sky separation).

Consider the case of points A, B, and C being arbitrary points on the sky. separation_AB is just a single number, the angle between A and B. So there's no way you can do separation_AB + separation_BC and get to the point C on the sky, because the 3d information has been lost in the separation operation. But a user might be tempted to do so, thinking it will just work. But that's not what "separation" means here, so the idea was to catch that confusion and prevent it from passing silently as a logic error.

Anyway, I'll create another issue to discuss this after Angles has become a Quantity subclass, and just merge this now.