Allow in-place modifications of representations #10210

mhvk · 2020-04-25T01:45:25Z

This is a follow-up of #9857 ~~(EDIT: and builds upon it, hence the many commits)~~, making also representations mutable. As commented there, representations themselves have no reason not to be mutable, since the behaviour of setting is well defined (in a way, they are just glorified quantities). Now that coordinates have become mutable, it makes little sense to keep the representations immutable.

~~I hope to follow this up with an DataInfo class such that representations can also be added to tables; without this, they would be second-class citizens.~~ EDIT: now added

fixes #6447 (as a side effect)

mhvk · 2020-04-25T20:09:21Z

@taldcroft - I think this is ready! Could you review? Especially the stuff related to table integration (i.e., info, small changes in operations.py and yaml.py).

taldcroft

This looks great from the table perspective, mostly minor comments.

CHANGES.rst

astropy/coordinates/representation.py

astropy/io/misc/yaml.py

astropy/coordinates/representation.py

taldcroft · 2020-04-27T10:28:05Z

astropy/coordinates/tests/test_representation.py

+    return result
+
+
+def representation_equal(rep1, rep2):


Makes me wonder if this should be __eq__ on the class.

Yes, it should be, but I thought to leave that for another PR (#10154??), in particular as I am not quite sure yet what to do with class mismatches.

astropy/table/tests/conftest.py

taldcroft · 2020-04-28T10:40:22Z

None of this should be considered a blocker for this PR, but there are outstanding issues related to serialization that should either be fixed (as long as you are in the code) or transferred to separate issues:

The differentials are still not being serialized at all in a SkyCoord:

>>> yaml.load(yaml.dump(sc))  # Loses the differential
<SkyCoord (ICRS): (ra, dec, distance) in (deg, deg, Mpc)
    [(270., 30., 10.)]>

>>> sc = SkyCoord([270] * u.deg, [30] * u.deg, [10] * u.Mpc, radial_velocity=[20] * u.km / u.s)
>>> t = Table([sc])
>>> t.write('junk.ecsv', overwrite=True)
>>> t2 = Table.read('junk.ecsv')
>>> t2['col0']
<SkyCoord (ICRS): (ra, dec, distance) in (deg, deg, Mpc)
    [(270., 30., 10.)]>
>>> t['col0']
<SkyCoord (ICRS): (ra, dec, distance) in (deg, deg, Mpc)
    [(270., 30., 10.)]
 (radial_velocity) in km / s
    [(20.,)]>

For a representation in a table the differential is being serialized as an encoded binary numpy instead of getting dropped in as a SerializedColumn. From a quick look we would need a deeper structure for the column name like col0.differentials.s.d_x. What is confusing is that the x differential is called v_x on input but d_x in the differential. No idea what's happening there.

>>> sc = SkyCoord(x=[1]*u.pc, y=[2]*u.pc, z=[3]*u.pc,
... ...      v_x=[4]*u.km/u.s, v_y=[5]*u.km/u.s, v_z=[6]*u.km/u.s,
... ...      representation_type=CartesianRepresentation,
... ...      differential_type=CartesianDifferential)
...
>>> t = Table([sc.frame.data])
>>> t.write('junk_repr.ecsv', overwrite=True)
>>> cat junk_repr.ecsv
# %ECSV 0.9
# ---
# datatype:
# - {name: col0.x, unit: pc, datatype: float64}
# - {name: col0.y, unit: pc, datatype: float64}
# - {name: col0.z, unit: pc, datatype: float64}
# meta: !!omap
# - __serialized_columns__:
#     col0:
#       __class__: astropy.coordinates.representation.CartesianRepresentation
#       __info__: {unit: pc}
#       differentials:
#         s: !astropy.coordinates.CartesianDifferential
#           d_x: !astropy.units.Quantity
#             unit: !astropy.units.Unit {unit: km / s}
#             value: !numpy.ndarray
#               buffer: !!binary |
#                 QUFBQUFBQUFFRUE9
#               dtype: float64
#               order: C
#               shape: !!python/tuple [1]
#           d_y: !astropy.units.Quantity
#             unit: !astropy.units.Unit {unit: km / s}
#             value: !numpy.ndarray
#               buffer: !!binary |
#                 QUFBQUFBQUFGRUE9
#               dtype: float64
#               order: C
#               shape: !!python/tuple [1]
#           d_z: !astropy.units.Quantity
#             unit: !astropy.units.Unit {unit: km / s}
#             value: !numpy.ndarray
#               buffer: !!binary |
#                 QUFBQUFBQUFHRUE9
#               dtype: float64
#               order: C
#               shape: !!python/tuple [1]
#       x: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         unit: &id001 !astropy.units.Unit {unit: pc}
#         value: !astropy.table.SerializedColumn {name: col0.x}
#       y: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         unit: *id001
#         value: !astropy.table.SerializedColumn {name: col0.y}
#       z: !astropy.table.SerializedColumn
#         __class__: astropy.units.quantity.Quantity
#         unit: *id001
#         value: !astropy.table.SerializedColumn {name: col0.z}
# schema: astropy-2.0
col0.x col0.y col0.z
1.0 2.0 3.0

mhvk · 2020-04-28T13:16:25Z

Thanks, I had started to notice that something was odd with differentials, but hadn't quite understood what was happening yet, so this is very helpful.

p.s. There also seems to be a strange problem with partial name clashes in serialized columns. If I can confirm that, I'll make it a separate issue, since it would need back-porting.

mhvk · 2020-04-29T00:04:45Z

@taldcroft - OK, with your fix, I now could get the round-tripping in FITS/HDF5/ECSV to work. I think I also addressed all your comments, except that I couldn't easily get SkyCoord with differentials to work (or rather, I got half-way, but found that even when there is just a radial_velocity, magic pm_ra_cosdec and pm_dec appear; since this PR is big as is, I thought I better leave it as a separate issue...).

adrn

So far have only had a quick look, but 👍 to the idea! No obvious big comments from first look, and may not have time to do a deeper dive before end of tomorrow...

mhvk · 2020-05-01T15:15:17Z

The failures are due to a strange but clearly temporary ability to fetch extension_helpers. I'll have another look at my own work here, and after that will restart CI just to be sure, and then merge.

Thanks, @adrn and @taldcroft for reviews. @eteq - time to speak up if you disagree with representations becoming settable is now...

taldcroft · 2020-05-01T15:26:50Z

I'm seeing the same weird CI problems.

pllim · 2020-05-01T15:37:50Z

Re: CI -- downgrade doctestplus to 0.5 + pytest 5.3

mhvk · 2020-05-01T16:00:05Z

@pllim - ah, good to know. Is it now fixed (is it you who restarted circle-ci? I don't seem to have permission myself)

pllim · 2020-05-01T16:01:08Z

I didn't touch it. 😄

bsipocz · 2020-05-01T18:37:59Z

ah, good to know. Is it now fixed (is it you who restarted circle-ci? I don't seem to have permission myself)

I think everyone with write access should have the power for circleCI. But you need to log-in with github to circleCI, and maybe also watch the repo.

There should not be any failure any more as part of an upstream hackery I reverted the doctestplus release that was causing the trouble.

mhvk · 2020-05-01T21:52:21Z

Darn, the windows run consistently hangs in test_io_fast_c_reader. Anybody any ideas?

pllim · 2020-05-01T22:18:26Z

windows run consistently hangs

Try run it with pytest-faulthandler.

…wargs And add test to ensure this doesn't recur.

mhvk · 2020-05-02T00:38:11Z

OK, travis is now running (and logging out and back in again to circle-CI meant I could indeed cancel).

mhvk · 2020-05-02T00:49:01Z

And now I have to wait for the travis time-out... I hope this isn't going to take a number of iterations!

mhvk · 2020-05-02T00:57:20Z

Hmm, that didn't help: still get

..\..\.tox\py37-test-alldeps\lib\site-packages\astropy\io\ascii\tests\test_c_reader.py x [ 17%]

..x.x.x.x.x.x.x.x.x.x.x.x...x................x.x.x.x.x.x.x.

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

The build has been terminated

From trying to count tests, I think it is

astropy/astropy/io/ascii/tests/test_c_reader.py

Line 660 in 986d5b7

def test_fast_reader():

so I'll go ahead and just try skipping this.

This is painful! I hate windows and hard-to-debug stuff due to compiled code!

mhvk · 2020-05-02T01:05:41Z

OK, I'm getting too annoyed and tired for my own good, so leaving this to tomorrow. It was approved already so hopefully this can be treated as "bug fix" (wish I even had a clue why the fast c reader cared about anything I did...)

pllim · 2020-05-02T05:03:02Z

p.s. I don't think remote data failures are related but you should double check.

mhvk · 2020-05-02T08:42:54Z

Wow, @pllim, that was some dedicated sleuthing! Thanks, so much!

As you note, the remote-date failures are all unrelated, so I'll go ahead and merge. It must still be friday somewhere...

taldcroft · 2020-05-02T15:10:51Z

@mhvk - this PR is breaking my local astropy. I get this from a clean build of astropy with this PR merged, and do not see a problem beforehand. This is with numpy 1.18.1.

In [1]: astro                                                                                                                                                 
astropy=4.1.dev1333+g27983b59c
/Users/aldcroft/git/astropy/astropy/table/column.py:1020: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
  result = getattr(super(), op)(other)
Downloading https://hpiers.obspm.fr/iers/bul/bulc/Leap_Second.dat
|========================================================================================================================| 1.3k/1.3k (100.00%)         0s
Downloading https://www.ietf.org/timezones/data/leap-seconds.list
|========================================================================================================================|  10k/ 10k (100.00%)         0s
WARNING: IERSStaleWarning: leap-second file is expired. [astropy.utils.iers.iers]

Darwin-19.2.0-x86_64-i386-64bit
Python 3.6.10 |Anaconda, Inc.| (default, Jan  7 2020, 15:01:53) 
[GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)]
Numpy 1.18.1
astropy 4.1.dev1494+g5ff936bd2
Scipy 1.4.1

Slightly OT, but the whole leap second download stuff still seems to be a source of really inscrutable problems. I really think it would be worth something like a config option to say "Don't ever try leap second downloads". I use astropy on mission-critical and non-networked computers, and I don't want it failing on that account.

pllim · 2020-05-02T15:15:10Z

Re: FutureWarning -- Hmm, why didn't the CI catch it?

taldcroft · 2020-05-02T15:56:59Z

Another manifestation:

(astropy) ➜  astropy git:(5ff936bd2) pytest astropy/io/ascii
==================================================================== test session starts =====================================================================
platform darwin -- Python 3.6.10, pytest-5.3.5, py-1.8.1, pluggy-0.13.1

Running tests with Astropy version 4.1.dev1494+g5ff936bd2.
Running tests in astropy/io/ascii.

Date: 2020-05-02T11:53:44

Platform: Darwin-19.2.0-x86_64-i386-64bit

Executable: /Users/aldcroft/miniconda3/envs/astropy/bin/python

Full Python Version: 
3.6.10 |Anaconda, Inc.| (default, Jan  7 2020, 15:01:53) 
[GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions: 
Numpy: 1.18.1
Scipy: 1.4.1
Matplotlib: 3.1.3
h5py: 2.10.0
Pandas: 1.0.0
Cython: not available
Scikit-image: 0.16.2
asdf: 2.6.0

Using Astropy options: remote_data: none.

rootdir: /Users/aldcroft/git/astropy, inifile: setup.cfg
plugins: arraydiff-0.3, remotedata-0.3.2, hypothesis-5.4.1, openfiles-0.4.0, xdist-1.31.0, filter-subpackage-0.1.1, forked-1.1.3, doctestplus-0.5.0, astropy-header-0.1.2, cov-2.8.1, asdf-2.6.0
collected 576 items / 1 error / 575 selected                                                                                                                 

=========================================================================== ERRORS ===========================================================================
_______________________________________________________ ERROR collecting astropy/io/ascii/__init__.py ________________________________________________________
astropy/time/core.py:2592: in update_leap_seconds
    table = iers.LeapSeconds.auto_open(files)
astropy/utils/iers/iers.py:1027: in auto_open
    warn('leap-second file is expired.', IERSStaleWarning)
E   astropy.utils.iers.iers.IERSStaleWarning: leap-second file is expired.

During handling of the above exception, another exception occurred:
astropy/io/ascii/__init__.py:7: in <module>
    from .core import (InconsistentTableError,
astropy/io/ascii/core.py:30: in <module>
    from astropy.table import Table
astropy/table/__init__.py:54: in <module>
    from .serialize import SerializedColumn, represent_mixins_as_columns
astropy/table/serialize.py:10: in <module>
    from astropy.coordinates import representation as coorep
astropy/coordinates/__init__.py:16: in <module>
    from .builtin_frames import *
astropy/coordinates/builtin_frames/__init__.py:29: in <module>
    from .fk5 import FK5
astropy/coordinates/builtin_frames/fk5.py:8: in <module>
    from astropy.coordinates import earth_orientation as earth
astropy/coordinates/earth_orientation.py:19: in <module>
    jd1950 = Time('B1950').jd
astropy/time/core.py:395: in __new__
    update_leap_seconds()
astropy/time/core.py:2597: in update_leap_seconds
    f"exception: {exc!r}", AstropyWarning)
E   astropy.utils.exceptions.AstropyWarning: leap-second auto-update failed due to the following exception: IERSStaleWarning('leap-second file is expired.',)
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
====================================================================== 1 error in 1.86s ======================================================================

taldcroft · 2020-05-02T15:57:42Z

So the question is if anyone else can reproduce this or if it's just me.

mhvk · 2020-05-02T18:29:35Z

I don't see how this could be related to this PR - but agree with the sentiment that the LeapSecond stuff is too fragile -- #9479. I'd very much appreciate help in trying to get it better!! While writing it, I found myself in an awful circular import morass between iers needing Time, and Time needing iers. I think for tests we should silence the stale-warning, since it is now turned into an error (which didn't use to be the case when I wrote the stuff).

It is possible to avoid any download with iers.config.auto_download=False (but this has to be done in the astropy config, since even importing iers already causes problems...).

taldcroft · 2020-05-03T11:08:50Z

astropy/table/serialize.py

 from .column import Column
 from .table import Table, QTable, has_info_class
 from astropy.units.quantity import QuantityInfo
+from astropy.coordinates import representation as coorep


After much flailing I finally figured out that this line is what is breaking things and causing all the confusing problems for me with warnings and test fails and leap seconds re-downloading every time. It is tricky because of circular imports, so the problem only shows up in a certain import order. You should be able to reproduce with a single import astropy.io.ascii without anything first. But import astropy.coordinates; import astropy.io.ascii is fine. And import astropy.table first gives /Users/aldcroft/git/astropy/astropy/table/column.py:1020: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison result = getattr(super(), op)(other)

This is latter warning is from the leap second stuff, and goes away if I remove this line here in serialize.py.

Darn, that must be because it imports Time - I'm sorry that I've caught all this mess, @pllim already had to move other imports inline to get things to pass.

I suggest a quick workaround to hardcode as strings all the representations currently in astropy.

OK, will make a PR. Clearly trying to be clever comes back to haunt oneself...

mhvk added coordinates Enhancement labels Apr 25, 2020

mhvk added this to the v4.1 milestone Apr 25, 2020

astropy-bot bot added the table label Apr 25, 2020

mhvk force-pushed the representations-mutable branch from 5d1ae83 to 6f60fa0 Compare April 25, 2020 11:02

mhvk requested review from adrn, eteq and taldcroft April 26, 2020 15:39

taldcroft reviewed Apr 27, 2020

View reviewed changes

mhvk mentioned this pull request Apr 27, 2020

Differential frame attributes can't be serialized to YAML #6447

Closed

taldcroft mentioned this pull request Apr 28, 2020

Add SpectralCoord class to astropy.coordinates #10185

Merged

13 tasks

mhvk mentioned this pull request Apr 28, 2020

Support Table fuzzy or cross-match join with SkyCoord, Quantity, Column #10169

Merged

3 tasks

mhvk force-pushed the representations-mutable branch from 30705ec to 0b1c18b Compare April 29, 2020 00:00

mhvk mentioned this pull request Apr 29, 2020

SkyCoord lose their differentials when written/read back as table columns #10226

Closed

mhvk force-pushed the representations-mutable branch from 0b1c18b to cc82180 Compare April 29, 2020 00:34

adrn approved these changes Apr 30, 2020

View reviewed changes

mhvk force-pushed the representations-mutable branch from ae8e4d4 to dde7305 Compare April 30, 2020 15:49

mhvk mentioned this pull request May 1, 2020

Implement __eq__ and __ne__ for coordinate classes #10154

Merged

mhvk added 5 commits May 1, 2020 20:34

Test that representations/differentials round-trip in FITS.

fef732b

Also add tests for representations/differentials to ecsv

5bbab5a

Also test representation/differential roundtrip in HDF5

6296a7a

Properly skip test if yaml not available.

98b7c25

Small fix ensuring extra kwargs raise error for reprs which pass on k…

f3650a5

…wargs And add test to ensure this doesn't recur.

mhvk force-pushed the representations-mutable branch from 3aaa7d6 to d271991 Compare May 2, 2020 00:34

mhvk added 2 commits May 1, 2020 20:57

Fix change-log entry after rebase

04f2a61

DO NOT MERGE: windows hang test

27983b5

mhvk force-pushed the representations-mutable branch from d271991 to 27983b5 Compare May 2, 2020 00:58

This comment has been minimized.

Sign in to view

Fix circular imports. Undo skip test.

e5ad1f6

mhvk merged commit 5ff936b into astropy:master May 2, 2020

mhvk deleted the representations-mutable branch May 2, 2020 08:43

mhvk mentioned this pull request May 2, 2020

Organize to avoid circular imports? #10270

Closed

taldcroft mentioned this pull request May 3, 2020

Cannot collect tests : pytest.PytestDeprecationWarning #10275

Closed

taldcroft reviewed May 3, 2020

View reviewed changes

astrofrog added a commit that referenced this pull request Aug 17, 2021

Backport components_equal from #10210

00bfe54

Uh oh!

Allow in-place modifications of representations #10210

Allow in-place modifications of representations #10210

Uh oh!

Conversation

mhvk commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Apr 25, 2020

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taldcroft Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

mhvk Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taldcroft commented Apr 28, 2020

Uh oh!

mhvk commented Apr 28, 2020

Uh oh!

mhvk commented Apr 29, 2020

Uh oh!

adrn left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 1, 2020

Uh oh!

taldcroft commented May 1, 2020

Uh oh!

pllim commented May 1, 2020

Uh oh!

mhvk commented May 1, 2020

Uh oh!

pllim commented May 1, 2020

Uh oh!

bsipocz commented May 1, 2020

Uh oh!

mhvk commented May 1, 2020

Uh oh!

pllim commented May 1, 2020

Uh oh!

mhvk commented May 2, 2020

Uh oh!

mhvk commented May 2, 2020

Uh oh!

mhvk commented May 2, 2020

Uh oh!

mhvk commented May 2, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pllim commented May 2, 2020

Uh oh!

mhvk commented May 2, 2020

Uh oh!

taldcroft commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented May 2, 2020

Uh oh!

taldcroft commented May 2, 2020

Uh oh!

taldcroft commented May 2, 2020

Uh oh!

mhvk commented May 2, 2020

Uh oh!

taldcroft May 3, 2020

Choose a reason for hiding this comment

Uh oh!

mhvk May 3, 2020

mhvk commented Apr 25, 2020 •

edited

Loading

taldcroft commented May 2, 2020 •

edited

Loading