For single columns, store standard info on Column. by mhvk · Pull Request #14 · taldcroft/astropy

mhvk · 2017-06-18T01:06:22Z

Really only to see whether it would work -- implemented too crappily to suggest using it directly. But with this, e.g.,

# %ECSV 0.9
# ---
# datatype:
# - {name: q, unit: m, datatype: float64, description: bla}
# meta: !!omap
# - __mixin_columns__:
#     q:
#       __class__: astropy.units.quantity.Quantity
#       value: !astropy.table.SerializedColumn {name: q}

I'm slightly wondering whether for single columns one could not just as well directly go through SerializedColumn, i.e., that this would be

q: !astropy.table.SerializedColumn {__class__: astropy.units.quantity.Quantity}

(name is obvious; value would be implied). That would certainly solve the annoying if statements in the current crappy implementation.

With that, I think it would also extend naturally to jd1 (class ndarray), and any regular dict would mean a level of recursion to get the SerializedColumn out (or another dict, and another level of recursion).

Anyway, really too tired -- I should have gone to sleep hours ago -- but this is very nice and I couldn't resist playing with it!

mhvk · 2017-06-18T10:48:52Z

@taldcroft - did you see this PR to your branch - it now doesn't merge any more, but one benefit it did have was not repeating the unit information when not necessary.

taldcroft · 2017-06-18T12:48:35Z

I had not seen this but will look now.

mhvk · 2017-06-23T00:31:48Z

@taldcroft - I rebaed this on your current version, made it quite a bit cleaner, and added recursive serialization (using SerializedColumn without name).

# %ECSV 0.9
# ---
# datatype:
# - {name: lon, unit: deg, datatype: float64}
# - {name: tm3.jd1, datatype: float64}
# - {name: tm3.jd2, datatype: float64}
# - {name: tm3.location.x, unit: km, datatype: float64}
# - {name: tm3.location.y, unit: km, datatype: float64}
# - {name: tm3.location.z, unit: km, datatype: float64}
# - {name: tm, datatype: float64}
# - {name: lat, unit: deg, datatype: float64}
# - {name: tm2, datatype: string}
# - {name: scc.x, datatype: float64}
# - {name: scc.y, datatype: float64}
# - {name: scc.z, datatype: float64}
# - {name: ang, unit: deg, datatype: float64}
# - {name: dt, datatype: float64}
# - {name: sc.ra, unit: deg, datatype: float64}
# - {name: sc.dec, unit: deg, datatype: float64}
# - {name: scd.ra, unit: deg, datatype: float64}
# - {name: scd.dec, unit: deg, datatype: float64}
# - {name: scd.distance, unit: m, datatype: float64}
# - {name: scd.obstime, datatype: string}
# - {name: q, unit: m, datatype: float64}
# - {name: el.x, unit: km, datatype: float64}
# - {name: el.y, unit: km, datatype: float64}
# - {name: el.z, unit: km, datatype: float64}
# meta: !!omap
# - __mixin_columns__:
#     ang:
#       __class__: astropy.coordinates.angles.Angle
#       value: !astropy.table.SerializedColumn {name: ang}
#     dt:
#       __class__: astropy.time.core.TimeDelta
#       format: jd
#       value: !astropy.table.SerializedColumn {name: dt}
#     el:
#       __class__: astropy.coordinates.earth.EarthLocation
#       ellipsoid: WGS84
#       x: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         value: !astropy.table.SerializedColumn {name: el.x}
#       y: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         value: !astropy.table.SerializedColumn {name: el.y}
#       z: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         value: !astropy.table.SerializedColumn {name: el.z}
#     lat:
#       __class__: astropy.coordinates.angles.Latitude
#       value: !astropy.table.SerializedColumn {name: lat}
#     lon:
#       __class__: astropy.coordinates.angles.Longitude
#       value: !astropy.table.SerializedColumn {name: lon}
#     q:
#       __class__: astropy.units.quantity.Quantity
#       value: !astropy.table.SerializedColumn {name: q}
#     sc:
#       __class__: astropy.coordinates.sky_coordinate.SkyCoord
#       dec: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.angles.Latitude
#         value: !astropy.table.SerializedColumn {name: sc.dec}
#       equinox: !astropy.time.Time {format: byear_str, in_subfmt: '*', jd1: 2400000.5,
#         jd2: 33281.92345905, out_subfmt: '*', precision: 3, scale: tai}
#       frame: fk4
#       obstime: !astropy.time.Time {format: jyear_str, in_subfmt: '*', jd1: 2400000.5,
#         jd2: 48074.625, out_subfmt: '*', precision: 3, scale: utc}
#       ra: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.angles.Longitude
#         value: !astropy.table.SerializedColumn {name: sc.ra}
#       representation: spherical
#     scc:
#       __class__: astropy.coordinates.sky_coordinate.SkyCoord
#       equinox: !astropy.time.Time {format: byear_str, in_subfmt: '*', jd1: 2400000.5,
#         jd2: 33281.92345905, out_subfmt: '*', precision: 3, scale: tai}
#       frame: fk4
#       obstime: !astropy.time.Time {format: jyear_str, in_subfmt: '*', jd1: 2400000.5,
#         jd2: 48074.625, out_subfmt: '*', precision: 3, scale: utc}
#       representation: cartesian
#       x: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         unit: &id001 !astropy.units.Unit {unit: ''}
#         value: !astropy.table.SerializedColumn {name: scc.x}
#       y: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         unit: *id001
#         value: !astropy.table.SerializedColumn {name: scc.y}
#       z: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         unit: *id001
#         value: !astropy.table.SerializedColumn {name: scc.z}
#     scd:
#       __class__: astropy.coordinates.sky_coordinate.SkyCoord
#       dec: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.angles.Latitude
#         value: !astropy.table.SerializedColumn {name: scd.dec}
#       distance: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.distances.Distance
#         value: !astropy.table.SerializedColumn {name: scd.distance}
#       equinox: !astropy.time.Time {format: byear_str, in_subfmt: '*', jd1: 2400000.5,
#         jd2: 33281.92345905, out_subfmt: '*', precision: 3, scale: tai}
#       frame: fk4
#       obstime: !astropy.table.SerializedColumn
#         __class__: astropy.time.core.Time
#         format: jyear_str
#         in_subfmt: '*'
#         out_subfmt: '*'
#         precision: 3
#         scale: utc
#         value: !astropy.table.SerializedColumn {name: scd.obstime}
#       ra: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.angles.Longitude
#         value: !astropy.table.SerializedColumn {name: scd.ra}
#       representation: spherical
#     tm:
#       __class__: astropy.time.core.Time
#       format: mjd
#       in_subfmt: '*'
#       location: !astropy.coordinates.earth.EarthLocation
#         ellipsoid: WGS84
#         x: !astropy.units.Quantity
#           unit: &id002 !astropy.units.Unit {unit: km}
#           value: 1.0
#         y: !astropy.units.Quantity
#           unit: *id002
#           value: 3.0
#         z: !astropy.units.Quantity
#           unit: *id002
#           value: 5.0
#       out_subfmt: '*'
#       precision: 5
#       scale: tai
#       value: !astropy.table.SerializedColumn {name: tm}
#     tm2:
#       __class__: astropy.time.core.Time
#       format: iso
#       in_subfmt: '*'
#       out_subfmt: '*'
#       precision: 5
#       scale: tai
#       value: !astropy.table.SerializedColumn {name: tm2}
#     tm3:
#       __class__: astropy.time.core.Time
#       format: mjd
#       in_subfmt: '*'
#       jd1: !astropy.table.SerializedColumn {name: tm3.jd1}
#       jd2: !astropy.table.SerializedColumn {name: tm3.jd2}
#       location: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.earth.EarthLocation
#         ellipsoid: WGS84
#         x: !astropy.table.SerializedColumn
#           __class__: astropy.units.quantity.Quantity
#           value: !astropy.table.SerializedColumn {name: tm3.location.x}
#         y: !astropy.table.SerializedColumn
#           __class__: astropy.units.quantity.Quantity
#           value: !astropy.table.SerializedColumn {name: tm3.location.y}
#         z: !astropy.table.SerializedColumn
#           __class__: astropy.units.quantity.Quantity
#           value: !astropy.table.SerializedColumn {name: tm3.location.z}
#       out_subfmt: '*'
#       precision: 5
#       scale: tai
# schema: astropy-2.0
lon tm3.jd1 tm3.jd2 tm3.location.x tm3.location.y tm3.location.z tm lat tm2 scc.x scc.y scc.z ang dt sc.ra sc.dec scd.ra scd.dec scd.distance scd.obstime q el.x el.y el.z
1.0 2451000.5 0.5 1.0 3.0 5.0 51000.5 1.0 "1998-07-06 12:00:00.00000" 0.998477438639 0.0174284885208 0.0523359562429 1.0 1.0 1.0 3.0 1.0 3.0 5.0 J1990.500 1.0 1.0 3.0 5.0
2.0 2451002.5 -0.5 2.0 4.0 6.0 51001.5 2.0 "1998-07-07 12:00:00.00000" 0.996956361194 0.0348144832826 0.0697564737441 2.0 2.0 2.0 4.0 2.0 4.0 6.0 J1990.500 2.0 2.0 4.0 6.0

taldcroft · 2017-06-23T02:01:13Z

Awesome, the dynamic @mhvk + @taldcroft duo continues!! 😄

I like this much better. Just got in from a flight and did not study the new code in much detail, but have looked at the serialized output and agree this is good.

About "so that Table can store EarthLocation as well", isn't that a lost cause? If you try to put an EarthLocation into Table then it gets downgraded to NdarrayMixin and loses all knowledge that it used to be an EarthLocation. Same goes for Angle => Column where the Angle class info is lost. In theory we could put add the right __mixin_columns__ meta at that point so it would magically round trip back to the right thing if transformed back to QTable. Was that the idea?

Anyway, please continue with your development and I will merge when you say you've finished.

taldcroft · 2017-06-23T02:29:53Z

Fun fact - this whole thing is predicated on an interesting "feature" of Table.add_column:

In [7]: q = [1,2,3]*u.m

In [8]: q.info.name='d'

In [9]: t.add_column(q)

In [10]: t
Out[10]: 
<Table length=3>
  a      b     c      d   
                      m   
int64 float64 str1 float64
----- ------- ---- -------
    1     1.0    c     1.0
    2     2.0    d     2.0
    3     3.0    e     3.0

In [11]: type(t['d'])
Out[11]: astropy.units.quantity.Quantity

Note that t is a Table!

taldcroft · 2017-06-23T10:18:44Z

With your approach maybe it makes sense to replace the key __mixin_columns__ with __serialized_columns__? I was never super-happy with the explicit reference to mixins, which is an astropy-specific concept.

mhvk · 2017-06-23T10:38:21Z

Yes, agreed, __serialized_columns__ is cleaner. Indeed, I wondered a bit about the nomenclature, now that it has become recursive: at some level, it would make sense to make all entries SerializedColumn instances. But then that just clutters the readable text and it is implied in having __serialized_columns__ in the first place.

As for add_column -- somehow I had not even wondered why the quantities stayed so happily in place in a Table... Maybe this means one can at least make it such that it does it for EarthLocation too - so it round-trips if you happen to have those in your table, but beyond that you're on your own.

taldcroft · 2017-06-23T11:04:27Z

I don't think EarthLocation can (or should) round-trip even if you manage to get it in there once. In the Table initialization code (init from list) it does this check:

            if (isinstance(col, np.ndarray) and len(col.dtype) > 1 and
                    not self._add_as_mixin_column(col)):
                col = col.view(NdarrayMixin)

For a Table object all those checks pass (the last one passes because EarthLocation is a Quantity subclass). The _add_as_mixin_column check is the bit responsible for ensuring that a Quantity in the Table generated within the ECSV code actually gets turned into a Column for the final table that gets returned to the user.

Agreed it's all a little confusing, but I'll stand by the original reasoning that got us here. So the upshot is to use QTable if you are using Quantity in your code, end of story.

mhvk · 2017-06-23T11:23:58Z

OK, I agree we should not try to make the table initializer recognize EarthLocation. But obviously it would be good if a Time with an array-valued EarthLoction is serialized properly (which it currently is).

But currently, if I write out with QTable, then any Quantity mixin columns get restored as Quantity. From what you write, I think you'd prefer that not to be the case, correct? This will need a bit of thought, since the recursion now means that the location of a Time gets temporarily stored in the output table, so we should not just adjust add_column.

One solution may be to work with TableColumns instead, and only do the translation to a [Q]Table at the very end. Anyway, I think I should do something on the velocity transforms now, so is it OK to hand over? I'll just push a few small changes I made, mostly the change in name, and to ensure that Time with array-valued location gets tested also for Table (which I think we should support).

taldcroft · 2017-06-23T12:39:03Z

I think the behavior should be exactly consistent with our current API and docs for casting from one class to another. I.e. QTable(qtable) round-trips perfectly, Table(qtable) converts Quantity (and subclasses) to Column + unit (with potential loss of information because the Quantity subclass is lost), QTable(table) will convert Column + unit to Quantity, etc.

So the ECSV is an intermediate representation that should be agnostic about what the final output will be. I think that is actually the case now, no?

mhvk · 2017-06-23T16:15:47Z

@taldcroft - currently, the .ecsv file is agnostic, but reading a file written by QTable with Table will have mixin Quantity columns (because of the behaviour of add_column that you noted above). Possibly the easiest solution is to have a table_class(out, copy_data=False) at the end; that way, storing an EarthLocation temporarily will still work.

taldcroft · 2017-06-24T02:15:18Z

@mhvk - hmm, it seems there is a slight problem. If one does t = ascii.read('mixins.ecsv', format='ecsv') then as you note that returns a hybrid Table that can have Quantity columns.
That's bad if the user gets hold of it.

However, in order that qt = QTable.read('mixins.ecsv', format='ascii.ecsv') works, we need the original ascii.read to return the hybrid table with all the Quantity subclasses intact. In the latter case conversion to QTable happens here, but only for subclasses of Table (not Table itself).

I thought about this for a few minutes and did not see any easy low-impact solution. Maybe you can?

Otherwise this is awesome and looking very close. After merging this I think there is just documentation related stuff.

taldcroft force-pushed the time-ecsv branch 2 times, most recently from 442bed0 to a9105d8 Compare June 19, 2017 02:02

Serialize recursively, avoiding duplication of info.

15dd7cf

mhvk force-pushed the pr-6181 branch from ac2fc49 to 15dd7cf Compare June 23, 2017 00:25

mhvk mentioned this pull request Jun 23, 2017

Allow round trip of Time, TimeDelta and SkyCoord through ECSV file astropy/astropy#6181

Merged

4 tasks done

Test array-valued Time.location for Table. Use __serialized_columns__.

aecc094

mhvk added 4 commits June 23, 2017 19:18

Ensure Longitude.wrap_angle is preserved.

02d9f06

Test angle mixins in table operations.

8e31f05

Unify info and data attrs in a single representation_as_dict_attrs.

99221e8

Make EarthLocation work as a mixin class.

3312cf4

taldcroft merged commit 1a32b21 into taldcroft:time-ecsv Jun 24, 2017

mhvk deleted the pr-6181 branch August 31, 2017 23:40

Navigation Menu

Repository navigation

For single columns, store standard info on Column. - #14

For single columns, store standard info on Column.#14
taldcroft merged 6 commits into
taldcroft:time-ecsvfrom
mhvk:pr-6181

mhvk commented Jun 18, 2017

Uh oh!

mhvk commented Jun 18, 2017

Uh oh!

taldcroft commented Jun 18, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mhvk commented Jun 18, 2017

Uh oh!

mhvk commented Jun 18, 2017

Uh oh!

taldcroft commented Jun 18, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 23, 2017

Uh oh!

mhvk commented Jun 23, 2017

Uh oh!

taldcroft commented Jun 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants