AltAz positional arguments are az, alt

This just costed me half a day.

Seriously. It's called AltAz. Why is it called AltAz. At least call it Horizontal or something else.

But if it's called AltAz, make sure the positional arguments are alt, az!!!

>>> from astropy.coordinates import AltAz
>>> a = AltAz('80d', '10d')
>>> a.alt.deg, a.az.deg
(10.0, 80.0)

@eteq

Yea, this gets me sometimes too. But: the context is that the class name is taken from the colloquialism ("alt-az", e.g., https://en.wikipedia.org/wiki/Altazimuth_mount), but the argument order is set to be consistent with the other classes (longitude-like angle first, so, azimuth). If we change the order of either of those, I personally think it will be even more confusing.

cc @eteq

How about making the arguments to AltAz have to be keywords now we are in the magic Python 3 land?

Yep, change it to AltAz(*, az=None, alt=None) This is really confusing.

Unfortunately due to the flexibility of BaseCoordinateFrame.__init__ this might not be so easy.

To me the real issue is that the docstring doesn't actually specify the order

A coordinate or frame in the Altitude-Azimuth system (Horizontal
coordinates).  Azimuth is oriented East of North (i.e., N=0, E=90 degrees).

This frame is assumed to *include* refraction effects if the ``pressure``
frame attribute is non-zero.

The frame attributes are listed under **Other Parameters**, which are
necessary for transforming from AltAz to some other system.

Parameters
----------
representation : `BaseRepresentation` or None
    A representation object or None to have no data (or use the other
    keywords)

az : `Angle`, optional, must be keyword
    The Azimuth for this object (``alt`` must also be given and
    ``representation`` must be None).
alt : `Angle`, optional, must be keyword
    The Altitude for this object (``az`` must also be given and
    ``representation`` must be None).

If I interpret the order of Parameters, the first argument should actually be representation. To me there is a definite 'bug' in documentation here. If the documentation was clearer, I think this would probably be a good enough fix. Unfortunately we can't change the order of the arguments at this point without breaking a lot of code. We could however emit a warning when out of bounds coordinates are passed (e.g. 100 deg to altitude).

@maxnoe

FWIW: I personally never rely on argument order because of exactly the confusion that @maxnoe raises here. I think a better practice in general is to just be explicit and always use ra=, dec= or alt=, az=.

But a few other things:

You already do get an error if you pass in, e.g., alt=100*u.deg because the value gets passed into the Latitude constructor, which fails if the value is not in (-90, 90) deg.
The order of the parameters in the docstring is actually ambiguous. You can pass in a representation object, e.g.,:

coord.AltAz(coord.SphericalRepresentation(lon=132*u.deg, lat=71*u.deg, distance=1.*u.pc))

The AltAz class is considered one of the "mid-tier" frame classes - SkyCoord was designed to have more of a user-friendly / forgiving API. See here

Unless you have a reason not to, I would generally use SkyCoord instead of the frame classes, e.g.,:

coord.SkyCoord(alt=12*u.deg, az=158*u.deg, frame='altaz')

But this is probably another documentation bug: we're inconsistent and ambiguous about our own usage of SkyCoord vs. the frame classes in the documentation examples.

@maxnoe

To me the real issue is that the docstring doesn't actually specify the order

In my mind the fact that az comes first and alt second does specify the order, or at least that was the intent. That said, I get that it's confusing, driven by the fact that there's no good way to specify a "tree" of parameters (e.g. "if just one, it's representation, otherwise it's az, alt"). I don't think that's so much a bug in the docs as much a bug in the way most humans read... (but could be wrong on that).

A question for @maxnoe, though: would it have helped if the docs were more explicit in that regard? I'm guessing from context that basically you thought the order was obvious (for good reason), so maybe you didn't even look at that part? (no judgement, just trying to understand how you got there!)

But this is probably another documentation bug: we're inconsistent and ambiguous about our own usage of SkyCoord vs. the frame classes in the documentation examples

Leads to a second question for @maxnoe : what lead you to AltAz instead of SkyCoord? There are plenty of cases where that's the right thing to do, but it's useful to know if this is the case to judge whether @adrn's point here in #6533 (comment) need prioritization.

On the underlying problem: I see the argument that it might make sense to make the keywords mandatory for this special case. We would have to go through a version or two with a deprecation warning... But I don't think this would be too hard to do: we can special-case AltAz itself with a specific initializer/__new__ (I think?)

@eteq

Coming back to this with fresh eyes, I see the argument for changing the name of the frame to Horizontal to remove ambiguity. What do you think @eteq?
https://en.wikipedia.org/wiki/Horizontal_coordinate_system

Yes, please do that 👍

The name change was shot down, so I think the solution to this is now to address #7709

Navigation Menu

Repository navigation

AltAz positional arguments are az, alt #6533

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

AltAz positional arguments are az, alt #6533

Description

Activity

adrn commented on Sep 11, 2017

Cadair commented on Sep 11, 2017

maxnoe commented on Sep 11, 2017

Cadair commented on Sep 11, 2017

astrofrog commented on Sep 11, 2017

adrn commented on Sep 11, 2017

eteq commented on Sep 12, 2017

adrn commented on Jul 15, 2018

maxnoe commented on Jul 15, 2018

adrn commented on Dec 3, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions