EarthLocation touchup's, mostly in documentation #2346

mhvk · 2014-04-21T20:06:16Z

Mostly dots at the ends of sentences in the documentation, but also removed __eq__, __ne__ (not needed anymore given #2328), and made ellipsoid a private properly, ensuring that it gets passed on when slices are made.

This is still pending discussion whether the arguments to the from_geodetic class method should have the long names; see #1928.

cdeil · 2014-04-21T20:55:56Z

astropy/coordinates/earth.py

Add a one-line description to this attribute.

Hmm, embarrassing that I missed the new one! But now corrected.

cdeil · 2014-04-21T21:08:25Z

The available ellipsoids are listed in two docstrings, but no reference is given. Maybe expose

ELLIPSOIDS = {'WGS84': 1, 'GRS80': 2, 'WGS72': 3}

via EarthLocation and give a reference to their definition in the docstring?

cdeil · 2014-04-21T21:17:22Z

You're very close to 100% test coverage for this class ... might as well cover these two lines: 220 and 255?

Does it make sense to have y=None and z=None as default values and then fail like this?
Maybe remove the =None default?

In [9]: EarthLocation.from_geocentric(42, unit='m')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-9-c378b2111f09> in <module>()
----> 1 EarthLocation.from_geocentric(42, unit='m')

/Users/deil/code/astropy2/astropy/coordinates/earth.py in from_geocentric(cls, x, y, z, unit)
     99         try:
    100             x = u.Quantity(x, unit, copy=False)
--> 101             y = u.Quantity(y, unit, copy=False)
    102             z = u.Quantity(z, unit, copy=False)
    103         except u.UnitsError:

/Users/deil/code/astropy2/astropy/units/quantity.py in __new__(cls, value, unit, dtype, copy, order, subok, ndmin)
    211                  isinstance(value.item(() if value.ndim == 0 else 0),
    212                             numbers.Number))):
--> 213             raise TypeError("The value must be a valid Python or "
    214                             "Numpy numeric type.")
    215 

TypeError: The value must be a valid Python or Numpy numeric type.

PS: sorry if I'm pointing out things that have been discussed already ... I haven't followed the coordinates discussions in the past months ... feel free to ignore me.

cdeil · 2014-04-21T21:34:54Z

This error message is a bit cryptic, no?

In [1]: from astropy.coordinates import EarthLocation

In [2]: loc = EarthLocation.from_geocentric(6000, 0, 0, unit='km')

In [3]: loc.geodetic
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-604a81e7db17> in <module>()
----> 1 loc.geodetic

/Users/deil/code/astropy2/astropy/units/quantity.pyc in __getattr__(self, attr)
    642                 "'{0}' object has no '{1}' member".format(
    643                     self.__class__.__name__,
--> 644                     attr))
    645 
    646         def get_virtual_unit_attribute():

AttributeError: 'EarthLocation' object has no 'geodetic' member

The reason is

In [4]: loc.to_geodetic()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-967038f3ca7c> in <module>()
----> 1 loc.to_geodetic()

/Users/deil/code/astropy2/astropy/coordinates/earth.pyc in to_geodetic(self, ellipsoid)
    192         ellipsoid = _check_ellipsoid(ellipsoid, default=self.ellipsoid)
    193         self_array = self.to(u.meter).view(self._array_dtype, np.ndarray)
--> 194         lon, lat, height = erfa_time.era_gc2gd(ELLIPSOIDS[ellipsoid],
    195                                                np.atleast_2d(self_array))
    196         return (Longitude(lon.squeeze() * u.radian, u.degree,

AttributeError: 'module' object has no attribute 'era_gc2gd'

How about adding an ellipsoid string option to from_geocentric ... otherwise I have to initialize this object in two lines if I want a different ellipsoid than the default, no?

mhvk · 2014-04-21T21:48:09Z

@cdeil - it is worrying how many things get caught by another review... Anyway, I think I implemented all your suggestions (except that I refer to erfa for the ellipsoids rather than expose the ELLIPSOIDS dictionary, which I felt was not that informative; did add a comment to ensure it is clearer why it is there, though).

mhvk · 2014-04-21T21:50:09Z

p.s. I did not have an ellipsoid entry to from_geocentric since it should have taken the class default. I think this will nearly always be what people want (and if not, they can either set a different one, or ask for a different one in to_geodetic). This should have worked in the first place....

cdeil · 2014-04-21T22:06:58Z

IMO explicit is better than implicit and the current initialisation API should be changed:

Initialization is first attempted assuming geocentric (x, y, z) coordinates are given; if that fails, another attempt is made assuming geodetic coordinates (longitude, latitude, height above a reference ellipsoid). Internally, the coordinates are stored as geocentric.

@mhvk Is renaming from_geocentric to __init__ an option you considered?

Otherwise this PR addresses all the comments I made, so 👍 to merging it, but someone that knows coordinates and the plans with this class well should still review it. @eteq?

mhvk · 2014-04-22T15:10:17Z

@cdeil - changing the initialisation goes a bit beyond the "touch-up" I hoped to do here -- we had quite a bit of discussion in #1928 and I would like to keep the option to initialise with geodetic coordinates. However, if there is an alternative that would easily allow that and is clearer, do let me know!. Short of that, would it help to explicitly list the initialisation parameters in the class docstring?

cdeil · 2014-04-23T10:36:37Z

My suggestion was to remove the current __new__ and rename the current from_geocentric to __init__. This would remove the need for the guesswork that's currently described in the class docstring "Initialization is first attempted ... if that fails, another attempt is made...". from_geodetic is nice and would remain as is, it just wouldn't be reachable implicitly via __new__.

For my taste there's too much magic going on trying to guess user inputs in __new__:

In [42]: EarthLocation(Quantity(1, 'm'), 2, 3)
Out[42]: <EarthLocation (1.0, 2.0, 3.0) m>

In [43]: EarthLocation(Quantity(1, 'deg'), 2, 3)
Out[43]: <EarthLocation (6373309.762299437, 111246.53570857966, 221104.65000960705) m>

In [44]: EarthLocation(1, 2, 3)
Out[44]: <EarthLocation (6373309.762299437, 111246.53570857966, 221104.65000960705) m>

In [45]: EarthLocation.from_geocentric(1, 2, 3)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-45-b69bbd56ca4d> in <module>()
----> 1 EarthLocation.from_geocentric(1, 2, 3)

/Users/deil/code/astropy/astropy/coordinates/earth.py in from_geocentric(cls, x, y, z, unit)
     94                 unit = x.unit
     95             except AttributeError:
---> 96                 raise TypeError("Geocentric coordinates should be Quantities "
     97                                 "unless an explicit unit is given.")
     98         else:

TypeError: Geocentric coordinates should be Quantities unless an explicit unit is given.

In [46]: EarthLocation.from_geocentric(Quantity(1, 'm'), 2, 3)
Out[46]: <EarthLocation (1.0, 2.0, 3.0) m>

In [47]: EarthLocation.from_geocentric(Quantity(1, 'deg'), 2, 3)
---------------------------------------------------------------------------
UnitsError                                Traceback (most recent call last)
<ipython-input-47-99a691625e10> in <module>()
----> 1 EarthLocation.from_geocentric(Quantity(1, 'deg'), 2, 3)

/Users/deil/code/astropy/astropy/coordinates/earth.py in from_geocentric(cls, x, y, z, unit)
    100 
    101         if unit.physical_type != 'length':
--> 102             raise u.UnitsError("Geocentric coordinates should be in "
    103                                "units of length.")
    104 

UnitsError: Geocentric coordinates should be in units of length.

In [48]: EarthLocation.from_geodetic(Quantity(1, 'deg'), 2, 3)
Out[48]: <EarthLocation (6373309.762299437, 111246.53570857966, 221104.65000960705) m>

There was a discussion on astropy-dev on how to input quantities, and I think the conclusion (at least the last people that spoke up) was to only accept Quantity objects as input, and not optionally float or arrays with an extra unit argument like you do here:
https://groups.google.com/d/topic/astropy-dev/ZDRZNUiOPBM/discussion

Playing with EarthLocation a bit more, I'm seeing a few dis-advantages to it being a Quantity, namely that it has a bunch of inappropriate ndarray methods (like cumsum) and claims to have others (like tostring) which raise a NotImplementedError:

In [49]: loc = EarthLocation(1, 2, 3)

In [52]: loc.
loc.T                loc.conj             loc.ediff1d          loc.isscalar         loc.nonzero          loc.shape            loc.tolist
loc.all              loc.conjugate        loc.ellipsoid        loc.item             loc.partition        loc.si               loc.tostring
loc.any              loc.copy             loc.equivalencies    loc.itemset          loc.prod             loc.size             loc.trace
loc.argmax           loc.ctypes           loc.fill             loc.itemsize         loc.ptp              loc.sort             loc.transpose
loc.argmin           loc.cumprod          loc.flags            loc.latitude         loc.put              loc.squeeze          loc.unit
loc.argpartition     loc.cumsum           loc.flat             loc.list             loc.ravel            loc.std              loc.value
loc.argsort          loc.data             loc.flatten          loc.longitude        loc.real             loc.strides          loc.var
loc.astype           loc.decompose        loc.from_geocentric  loc.max              loc.repeat           loc.sum              loc.view
loc.base             loc.diagonal         loc.from_geodetic    loc.mean             loc.reshape          loc.swapaxes         loc.x
loc.byteswap         loc.diff             loc.geocentric       loc.min              loc.resize           loc.take             loc.y
loc.cgs              loc.dot              loc.geodetic         loc.nansum           loc.round            loc.to               loc.z
loc.choose           loc.dtype            loc.getfield         loc.nbytes           loc.searchsorted     loc.to_geocentric    
loc.clip             loc.dump             loc.height           loc.ndim             loc.setfield         loc.to_geodetic      
loc.compress         loc.dumps            loc.imag             loc.newbyteorder     loc.setflags         loc.tofile           

In [52]: loc.shape
Out[52]: ()

In [54]: loc.tostring()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-54-7f59cb2cd873> in <module>()
----> 1 loc.tostring()

/Users/deil/code/astropy/astropy/units/quantity.py in tostring(self, order)
   1003 
   1004     def tostring(self, order='C'):
-> 1005         raise NotImplementedError("cannot write Quantities to string.  Write "
   1006                                   "array with q.value.tostring(...).")
   1007 

NotImplementedError: cannot write Quantities to string.  Write array with q.value.tostring(...).

But @mhvk, I completely agree that these fundamental questions whether EarthLocation should be a Quantity or how it should be initialised should not hold up this "touchup" PR, so 👍.

mhvk · 2014-04-23T15:16:06Z

@cdeil - I agree that subclassing from Quantity is not ideal, but this should disappear soon -- there now is quite a bit of work on redoing coordinates [1], and the idea is that once that goes in, EarthLocation will subclass from CartesianRepresentation.

Your comments got me thinking again about __new__, as I both see the advantage of having a clear, well-defined interface, and of code being able to deal with any unambiguous input. For the latter, insisting on Quantity input would help in some way, although I find it pleasing that in Time, the only place where EarthLocation is currently used [2], one can set location=('45d', '60d'), and it will be understood. Internally to Time, I would prefer not having to write a location parser, but arguably it would work just as well if there was an EarthLocation.from_tuple class method or so that would do the guessing (or treat a tuple as a first parameter differently in __new__).

[1] https://github.com/eteq/astropy/tree/coordinates-ape5
[2] http://docs.astropy.org/en/latest/time/index.html#location

eteq · 2014-04-23T22:32:41Z

This looks fine and the tests are passing, so I'm going to merge this as a "touchup". But I do agree with some of @cdeil's points, so @cdeil, can you create a new issue and discuss it there so that the discussion doesn't get lost in merging this?

EarthLocation touchup's, mostly in documentation

cdeil · 2014-04-23T22:45:15Z

@eteq I'll leave the creation of new issues / PRs on this to you and @mhvk ... I really haven't followed the coordinates work and don't think it would be very useful for me to make an issue trying to describe some small part of this work. But I promise to complain again before if EarthLocation initialisation hasn't been simplified before the Astropy 0.4 release, OK? :-)

@mhvk Thanks for listening to my comments from the peanut gallery.

eteq · 2014-04-23T23:29:32Z

Sounds like a plan!

mhvk · 2014-04-24T04:17:44Z

@cdeil, @eteq - I've looked a little more and now are tending the other way again, thinking that really it is nice if one can initialize with angles directly; it is after all pretty unambiguous, and much of coordinates seem to work similarly (though this is changing somewhat). But I wondered if at least an update of the class docstring would help, as it would explain the parameters expected by default. What do you think of the following? (Note that unlike, e.g., FK4 [1], I do not explicitly list the other possibility, but refer to the class method.)

[1] http://docs.astropy.org/en/latest/api/astropy.coordinates.FK4.html#astropy.coordinates.FK4

    """
    Location on Earth.

    Locations are initialised with (x, y, z) coordinates by default; if that
    fails, another attempt is made assuming geodetic coordinates (longitude,
    latitude, height above a reference ellipsoid).

    Internally, the coordinates are stored as geocentric.  They can be
    converted to geodetic using the `to_geodetic` method.

    Parameters
    ----------
    x, y, z : `~astropy.units.Quantity` or array-like
        Cartesian coordinates.  If not quantities, ``unit`` should be given.
    unit : `~astropy.units.UnitBase` object or None
        Physical unit of the coordinate values.  If ``x``, ``y``, and/or ``z``
        are quantities, they will be converted to this unit.

    Raises
    ------
    TypeError
        If initialisation fails.

    Notes
    -----
    To initialize with a specific type of coordinates, use the corresponding
    class method (`from_geocentric` and `from_geodetic`) or initialize the
    arguments by name (``x``, ``y``, ``z`` for geocentric; ``lon``, ``lat``,
    ``height`` for geodetic).  See the class methods for details.

    For conversion to and from geodetic coordinates, the ERFA routines
    ``gc2gd`` and ``gd2gc`` are used.  See https://github.com/liberfa/erfa
    """

eteq · 2014-04-24T05:20:21Z

@mhvk - I like the idea, but its a bit confusing that you say x, y, and z in the Parameters section, as that implies only geocentric will work, but if I'm understanding right that's not what you intend...?

Also, in this form unit becomes more confusing, because it makes no sense in the geodetic case, given that angle and height units are not the same...?

cdeil · 2014-04-24T06:07:07Z

@mhvk As I said, I'd prefer to resolve the ambiguities in the initialisation by only allowing initialisation from geodetic coordinates via from_geodetic. The problem with the current initialisation is that it's not always obvious (see the examples I gave above) if from_geodetic or from_geocentric will be called.
Like @eteq I think that only listing x, y, z in the Parameters section would be confusing.

Make ellipsoid a property with a setter; add tests; use _new_view

bdb4946

mhvk mentioned this pull request Apr 21, 2014

EarthLocation class for Time #1928

Merged

cdeil reviewed Apr 21, 2014
View reviewed changes

Add docstring to ellipsoid

78c78fd

Further tests/changes following comments by @cdeil

9af51dc

astrofrog added time labels Apr 22, 2014

astrofrog added this to the v0.4.0 milestone Apr 22, 2014

astrofrog added the ready-for-final-review label Apr 22, 2014

cdeil mentioned this pull request Apr 23, 2014

SEP describing the use of Astropy Quantities in the SunPy API sunpy/sunpy-SEP#7

Merged

eteq added a commit that referenced this pull request Apr 23, 2014

Merge pull request #2346 from mhvk/time-location-doc-touchup

32e2805

EarthLocation touchup's, mostly in documentation

eteq merged commit 32e2805 into astropy:master Apr 23, 2014

eteq added the Affects-dev label Apr 23, 2014

mhvk deleted the time-location-doc-touchup branch April 24, 2014 04:09

embray removed the Ready-for-final-review label Jan 13, 2015

Uh oh!

EarthLocation touchup's, mostly in documentation #2346

EarthLocation touchup's, mostly in documentation #2346

Uh oh!

Conversation

mhvk commented Apr 21, 2014

Uh oh!

cdeil Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

mhvk Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

cdeil commented Apr 21, 2014

Uh oh!

cdeil commented Apr 21, 2014

Uh oh!

cdeil commented Apr 21, 2014

Uh oh!

mhvk commented Apr 21, 2014

Uh oh!

mhvk commented Apr 21, 2014

Uh oh!

cdeil commented Apr 21, 2014

Uh oh!

mhvk commented Apr 22, 2014

Uh oh!

cdeil commented Apr 23, 2014

Uh oh!

mhvk commented Apr 23, 2014

Uh oh!

eteq commented Apr 23, 2014

Uh oh!

cdeil commented Apr 23, 2014

Uh oh!

eteq commented Apr 23, 2014

Uh oh!

mhvk commented Apr 24, 2014

Uh oh!

eteq commented Apr 24, 2014

Uh oh!

cdeil commented Apr 24, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants