alpha not in [0, 2\pi)

[chris-hld]

https://github.com/sfstoolbox/sfs-python/blob/ac86493b1a07775731c46d4d1701660a6e83258d/sfs/util.py#L125

alpha is not in [0, 2\pi) but in [-pi, pi]. This can turn out as a bug when using it.

We could add alpha[alpha < 0] += 2*np.pi or something even smarter.

[chris-hld]

This is be better and should be faster: alpha = (alpha + 2*np.pi) % (2*np.pi)

[mgeier]

Why should it be the one range and not the other?

Is there any math textbook or relevant software that uses your suggested behavior for azimuth?

This can turn out as a bug when using it.

But that's a bug in the other code then, isn't it? Any function taking an angle in radians should simply do The Right Thing when given an "out-of-range" value.

A function that only takes values from -pi to pi is equally broken.

[chris-hld]

It says in the documentation and theory. I think it makes sense to provide it then.

[chohner]

I think Chris is referring to https://github.com/sfstoolbox/sfs-python/blob/ac86493b1a07775731c46d4d1701660a6e83258d/sfs/util.py#L104 Alpha is specified to be returned in range [0, 2pi) but np.arctan2() returns in the range [-pi/2, pi/2] (see https://docs.scipy.org/doc/numpy-1.14.0/reference/generated/numpy.arctan2.html)

[mgeier]

OK, so probably the documentation should be changed?

[hagenw]

Interesting discussion. I agree that all functions should work with out of range values as input.

For return values I would expect that they are within the range used throughout the toolbox. At the moment this would be [0, 2pi) as this is used in the theory at the moment, see http://sfstoolbox.org/en/2.9.0/#coordinate-system. The same convention is used by Ahrens 2012 and Schultz 2016.

So maybe, we should not change the documentation, but the return value.

[mgeier]

@hagenw Thanks for the references, that's very helpful!

Ahrens 2012 in turn references this: Weisstein, E. W. (2002). CRC concise encyclopedia of mathematics. London: Chapman and Hall/CRC. I don't have access to this book, so I couldn't check it out.

I agree that the range [0, 2pi) makes sense in a theoretical math context. I'm not sure, however, if it also makes sense in a numeric implementation. The numpy.arctan2() has a nice implementation of some "special" values (as shown in the documentation linked by @chohner), which we would most likely destroy if we add our own number crunching.

So I would still be interested how other software does this!

[fs446]

Here is a reference from Wolfram, which due to collaboration of the parties might be consistent with the Weisstein book (however I did not check): http://functions.wolfram.com/PDF/ArcTan2.pdf

The implementation of numpy.arctan2 seems to be checked against standardization efforts of IEEE, NIST (e.g. by Weisstein). I'd strongly suggest not to implement our own handling only to be consistent with a theoretical return value [0, 2pi) that looks nice in analytic calculus. If one ever needs precisely [0, 2pi) returned from arctan2 for whatever reason, one should implement and precisely validate this for this special application. In our problems the standard return values can be straightforwardly used for subsequent calculations. Please indicate where this handling fails.

IMO much more important would be to reconsider i) the naming alpha and beta to a more standardized version and ii) the usage of 'elevation' more precisely. We use the polar/colatidue angle, thus 'elevation' is misleading.

[mgeier]

@fs446 Can you please make separate issues/PRs for i) and ii)?

[mgeier]

Based on the discussions on #73, we should change the docs instead of changing the implementation.

Would anyone like to make those documentation changes?

[chris-hld]

I can take care of this. I'll include i), ii) and updating the test.

[mgeier]

Thanks @chris-hld! I think re-naming the parameters might need some discussion, so it's probably better to do this in a separate PR.

[mgeier]

This was closed with #109, thanks @chris-hld.