Trial implementation of __repr__ and __str__ by mhvk · Pull Request #13 · eteq/astropy

mhvk · 2014-04-23T05:07:24Z

To have repr and str produce at least something, I combined the looks of recarray and Quantity, a bit like happens now for EarthLocation. The current implementation is far from fast & perfect, but suffices perhaps to show what it will look like (also, it seems impossible to raise an issue, so one has to do a PR...).

cc @eteq, @taldcroft, @astrofrog, @Cadair, @adrn

In action (note how the angle units get lost; bit of a pity, but not essential for now):

In [1]: from astropy.coordinates.representation import *

In [2]: import numpy as np; import astropy.units as u

In [3]: c = SphericalRepresentation(np.arange(0, 360, 90) * u.deg, np.arange(-90, 90, 45) * u.deg, np.arange(1,5)*u.pc)

In [4]: c
Out[4]: 
<SphericalRepresentation [(0.0, -90.0, 1.0),(90.0, -45.0, 2.0),
                          (180.0, 0.0, 3.0),(270.0, 45.0, 4.0)] (deg,deg,pc)>

In [5]: c.to_cartesian()
Out[5]: 
<CartesianRepresentation [(6.123233995736766e-17, 0.0, -1.0),
                          (8.659560562354934e-17, 1.4142135623730951, -1.414213562373095),
                          (-3.0, 3.6739403974420594e-16, 0.0),
                          (-5.19573633741296e-16, -2.8284271247461903, 2.82842712474619)] pc>

In [6]: c.represent_as(UnitSphericalRepresentation)
Out[6]: 
<UnitSphericalRepresentation [(0.0, -1.5707963267948966),
                              (1.5707963267948966, -0.7853981633974483),
                              (3.141592653589793, 0.0),
                              (4.71238898038469, 0.7853981633974483)] rad>

In [7]: c.represent_as(CylindricalRepresentation)
Out[7]: 
<CylindricalRepresentation [(6.123233995736766e-17, 0.0, -1.0),
                            (1.4142135623730951, 1.5707963267948966, -1.414213562373095),
                            (3.0, 3.141592653589793, 0.0),
                            (2.8284271247461903, -1.5707963267948968, 2.82842712474619)] (pc,rad,pc)>

n [8]: c.represent_as(PhysicsSphericalRepresentation)
Out[8]: 
<PhysicsSphericalRepresentation [(0.0, 180.0, 1.0),(90.0, 135.0, 2.0),
                                 (180.0, 90.0, 3.0),(270.0, 45.0, 4.0)] (deg,deg,pc)>

In [9]: print(c)
[(0.0, -90.0, 1.0) (90.0, -45.0, 2.0) (180.0, 0.0, 3.0) (270.0, 45.0, 4.0)] (deg,deg,pc)

In [10]: print(c.represent_as(UnitSphericalRepresentation))
[(0.0, -1.5707963267948966) (1.5707963267948966, -0.7853981633974483)
 (3.141592653589793, 0.0) (4.71238898038469, 0.7853981633974483)] rad

eteq · 2014-04-23T21:32:24Z

Can this be changed to something slightly more descriptive like array_names or data_names or similar? I know you have it as private, but anyone wanting to subclass will need to implement it, so it can't be private.

@astrofrog or @Cadair, does the use of _names seem fine to you? Or was there some other way you had in mind for how to sniff out the names of the data attributes?

eteq · 2014-04-23T21:36:39Z

I like this in general, but I'm confused by the output you're showing: why are there four entries for each, when there are only three coordinates?

eteq · 2014-04-23T21:39:06Z

Oh wait, I get it... it's a list of each of the coordinates, not a list of the elements of the arrays.

So that are we ok with the fact that this is in contrast to the low-level classes _repr_, where it looks like <ICRS ra=[...] deg, dec=[...] deg, distance=[...] pc> ? I guess I can see the merits of both, so maybe this is a good thing...

mhvk · 2014-04-23T21:51:26Z

@eteq, others: the implementation really was extremely quick & dirty -- I would definitely like to clean it up before you give detailed comments!
Instead, the first thing to decide is whether we like this type of output, i.e., whetehr the coordinates should be as tuples together, or rather as separate arrays. I'm sorry that I didn't look at the precedent, and used the reverse logic, that mostly one is interested in all coordinates of a given entry rather the set of RA, etc. Essentially, I think of these classes as being some equivalent of a table, which one would also look at row-by-row. Indeed, I considered trying to (ab)use the printing mechanism of Table, which would be quite neat for displaying the units, etc.

eteq · 2014-04-23T21:55:43Z

Hmm, well, as I said, I see the merits of both, so I'm willing to accept this way for the representations, and the other way for the low-level classes (there it makes more sense anyway, because they actually have useful names, and the frame attributes have to be in a similar form). But other opinions would be to have registered before a final decsion.

eteq · 2014-04-23T21:58:08Z

Oh, and FYI in case it's an oversight, it seems to fail for scalars, @mhvk

In [9]: r=representation.SphericalRepresentation(1*u.deg,2*u.deg,3*u.kpc)

In [10]: repr(r)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-10-199998cc1a69> in <module>()
----> 1 repr(r)

/Users/erik/src/astropy/build/lib.macosx-10.9-x86_64-2.7/astropy/coordinates/representation.pyc in __repr__(self)
     97     def __repr__(self):
     98         prefixstr = '<' + self.__class__.__name__ + ' '
---> 99         arrstr = np.array2string(self._values, separator=',',
    100                                  prefix=prefixstr)
    101 

/Users/erik/src/astropy/build/lib.macosx-10.9-x86_64-2.7/astropy/coordinates/representation.pyc in _values(self)
     75     @property
     76     def _values(self):
---> 77         allcomp = np.array([getattr(self, _name) for _name in self._names])
     78         return np.rollaxis(allcomp, 0, len(allcomp.shape)).copy().view(
     79             ','.join(['f8'] * len(self._names))).squeeze()

ValueError: setting an array element with a sequence.

mhvk · 2014-04-24T02:12:20Z

@eteq - the single elements are working now too. I tried a slightly different approach to repr, listing the components. Curious to hear whether you think it is better. Ideally, the different levels of the classes have similarly looking repr and str. (A different approach is further down.)

In [2]: c = SphericalRepresentation(90 * u.deg, 45 * u.deg, 5*u.pc)

In [3]: c
Out[3]: 
<SphericalRepresentation components=(lon, lat, distance) units=(deg, deg, pc)
    (90.0, 45.0, 5.0)>

In [4]: c.to_cartesian()
Out[4]: 
<CartesianRepresentation components=(x, y, z) unit=pc
    (2.1648901405887333e-16, 3.5355339059327378, 3.5355339059327373)>

In [5]: c = SphericalRepresentation(np.arange(0, 360, 90) * u.deg, np.arange(-90, 90, 45) * u.deg, np.arange(1,5)*u.pc)

In [6]: c
Out[6]: 
<SphericalRepresentation components=(lon, lat, distance) units=(deg, deg, pc)
    [(0.0, -90.0, 1.0), (90.0, -45.0, 2.0), (180.0, 0.0, 3.0),
     (270.0, 45.0, 4.0)]>

In [7]: c.to_cartesian()
Out[7]: 
<CartesianRepresentation components=(x, y, z) unit=pc
    [(6.123233995736766e-17, 0.0, -1.0),
     (8.659560562354934e-17, 1.4142135623730951, -1.414213562373095),
     (-3.0, 3.6739403974420594e-16, 0.0),
     (-5.19573633741296e-16, -2.8284271247461903, 2.82842712474619)]>

And the different approach, which I felt was rather ugly, was:

<SphericalRepresentation
 (lon, lat, distance) = [(0.0, -90.0, 1.0),(90.0, -45.0, 2.0),
                        (180.0, 0.0, 3.0),(270.0, 45.0, 4.0)] (deg, deg, pc)>

eteq · 2014-04-24T18:15:53Z

I like this version, @mhvk, now that I've gotten used to the idea.

Two things:

Can you add a mention of component_names to the "Notes" section of BaseRepresentation? That's basically a "guide to subclassing", so you should mention that component_names is needed.
This is what I see when I use a scalar:

In [28]: r=coo.SphericalRepresentation(1*u.deg,2*u.deg,8*u.kpc)

In [29]: r
Out[29]: 
<SphericalRepresentation components=(lon, lat, distance) units=(deg, deg, kpc)
    (1.0, 2.0, 8.0)>

In [30]: print str(r)
(1.0, 2.0, 8.0) (deg, deg, kpc)

How would you feel about special-casing the scalar version to instead give

>>> print repr(r)
<SphericalRepresentation lon=1.0 deg, lat= 2.0 deg, distance=8.0 kpc>

and

>>> print str(r)
(1.0 deg, 2.0 deg, 8.0 kpc)
```?

eteq · 2014-04-24T18:16:47Z

Oh, and it will be easy to adjust the frame classes to make a similar-looking repr by reusing this code, so that's a good thing (TM).

mhvk · 2014-04-24T19:13:23Z

@eteq - OK, like that idea, so next iteration. Also realised that once component names are known, __getitem__ can be defined in the base. (In principle, should be able to get self.components automatically -- certainly, they are all that is in __dict__, but I'm not sure to what extend one can count on the order being right).

Anyway, repr and str now look as follows:

from astropy.coordinates.representation import *
import astropy.units as u, numpy as np
c = SphericalRepresentation(np.arange(0, 360, 90) * u.deg, np.arange(-90, 90, 45) * u.deg, np.arange(1,5)*u.pc)
In [8]: c
Out [8]:
<SphericalRepresentation (lon, lat, distance) in (deg, deg, pc)
    [(0.0, -90.0, 1.0), (90.0, -45.0, 2.0), (180.0, 0.0, 3.0),
     (270.0, 45.0, 4.0)]>
In [9]: c[0]
Out[9]: <SphericalRepresentation lon=0.0 deg, lat=-90.0 deg, distance=1.0 pc>
In [10]: c.to_cartesian()
<CartesianRepresentation (x, y, z) in m
    [(19.69615506024416, 0.0, 3.4729635533386065),
     (20.611031214333146, 0.35976688808370433, 4.006988902907441),
     (21.50613827217025, 0.751010897260111, 4.574057197990705),
     (22.37979866293106, 1.1728755487257918, 5.173874249908895),
     (23.230371231688338, 1.624425800496205, 5.806125494392026),
     (24.056254672476456, 2.1046495707342294, 6.4704761275630185),
     (24.85589089852694, 2.612459404736762, 7.166571251241979),
     (25.627768343839918, 3.146694283019743, 7.894036027513893),
     (26.37042519035925, 3.7061215677100585, 8.652475842498527),
     (27.08245251513146, 4.289439084224926, 9.441476479257544)]>
In [11]: c[0].to_cartesian()
Out[11]: <CartesianRepresentation x=6.123233995736766e-17 pc, y=0.0 pc, z=-1.0 pc>
In [12]: print(c)
[(0.0, -90.0, 1.0) (90.0, -45.0, 2.0) (180.0, 0.0, 3.0) (270.0, 45.0, 4.0)] (deg, deg, pc)
In [13]: print(c[0])
(0.0 deg, -90.0 deg, 1.0 pc)
In [14]: print(c.to_cartesian())
[(6.123233995736766e-17, 0.0, -1.0)
 (8.659560562354934e-17, 1.4142135623730951, -1.414213562373095)
 (-3.0, 3.6739403974420594e-16, 0.0)
 (-5.19573633741296e-16, -2.8284271247461903, 2.82842712474619)] pc
In [15]: print(c[0].to_cartesian())
(6.123233995736766e-17, 0.0, -1.0) pc

eteq · 2014-04-24T20:10:01Z

@mhvk - I like how that looks, but I'm seeing a new error, which I demonstrate below. I think the problem is that np.dtype([(name, float]) doesn't work if name is unicode, and that's happening for some reason (but apparently not for you? Perhaps it's a platform thing...)

In [15]: rc=coo.CartesianRepresentation(1*u.kpc,2*u.kpc,3*u.kpc)

In [16]: rc
Out[16]: <CartesianRepresentation x=1.0 kpc, y=2.0 kpc, z=3.0 kpc>

In [17]: rc2=coo.CartesianRepresentation(1*u.kpc,2*u.kpc,[3,4]*u.kpc)

In [18]: rc2
Out[18]: <repr(<astropy.coordinates.representation.CartesianRepresentation at 0x1066e9fd0>) failed: TypeError: data type not understood>

In [19]: repr(rc2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-19-b583fdedad3f> in <module>()
----> 1 repr(rc2)

/Users/erik/src/astropy/build/lib.macosx-10.9-x86_64-2.7/astropy/coordinates/representation.pyc in __repr__(self)
    131         else:
    132             prefixstr = '    '
--> 133             arrstr = np.array2string(self._values, separator=', ',
    134                                      prefix=prefixstr)
    135 

/Users/erik/src/astropy/build/lib.macosx-10.9-x86_64-2.7/astropy/coordinates/representation.pyc in _values(self)
     92                             for component in self.components])
     93         dtype = np.dtype([(component, np.float)
---> 94                           for component in self.components])
     95         return (np.rollaxis(allcomp, 0, len(allcomp.shape))
     96                 .copy().view(dtype).squeeze())

TypeError: data type not understood

eteq · 2014-04-25T05:23:21Z

Alright, this now seems fine to me @mhvk - if no one else voices an opion, I'll merge it tomorrow.

eteq · 2014-04-25T21:07:09Z

Alright, I'm going to go ahead and merge this now. Thanks @mhvk!

Implementation of representations' __repr__ and __str__

Lazyproperty for cache / easier scale setting in __add__, __sub__

Trial implementation of __repr__ and __str__

c2c944b

eteq reviewed Apr 23, 2014
View reviewed changes

Use component_names, make scalars work, different repr

adc8d65

Adapt better to scalars; write __getitem__ using self.components

15b7bbc

Add str() in dtype to work around pre-unicode numpy/python2

67dc5dc

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

Merge pull request #13 from mhvk/ape5-repr-str

9258c27

Implementation of representations' __repr__ and __str__

eteq merged commit 9258c27 into eteq:coordinates-ape5 Apr 25, 2014

eteq mentioned this pull request Apr 25, 2014

High-level SkyCoordinate class (WIP) #17

Merged

mhvk deleted the ape5-repr-str branch September 26, 2014 16:40

eteq pushed a commit that referenced this pull request Jan 12, 2016

Merge pull request #13 from mhvk/pr4422

15329ee

Lazyproperty for cache / easier scale setting in __add__, __sub__

Navigation Menu

Repository navigation

Trial implementation of repr and str - #13

Trial implementation of repr and str#13
eteq merged 4 commits into
eteq:coordinates-ape5from
mhvk:ape5-repr-str

mhvk commented Apr 23, 2014

Uh oh!

eteq Apr 23, 2014

Uh oh!

eteq commented Apr 23, 2014

Uh oh!

eteq commented Apr 23, 2014

Uh oh!

mhvk commented Apr 23, 2014

Uh oh!

eteq 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!

eteq commented Apr 24, 2014

Uh oh!

mhvk commented Apr 24, 2014

Uh oh!

eteq commented Apr 24, 2014

Uh oh!

eteq commented Apr 25, 2014

Uh oh!

eteq commented Apr 25, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mhvk commented Apr 23, 2014

Uh oh!

eteq Apr 23, 2014

Choose a reason for hiding this comment

Uh oh!

eteq commented Apr 23, 2014

Uh oh!

eteq commented Apr 23, 2014

Uh oh!

mhvk commented Apr 23, 2014

Uh oh!

eteq 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!

eteq commented Apr 24, 2014

Uh oh!

mhvk commented Apr 24, 2014

Uh oh!

eteq commented Apr 24, 2014

Uh oh!

eteq commented Apr 25, 2014

Uh oh!

eteq commented Apr 25, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants