SkyCoord/Baseframe shape changes by mhvk · Pull Request #5254 · astropy/astropy

mhvk · 2016-08-22T21:31:39Z

Now that representations can change shape, etc. (#4123), it is time to move up to also allow this for frames ( and ~~eventually~~ coordinates~~)~~.

For frames, one has to ensure that any frame attributes are reshaped if needed. To get this to work, before anything else, all attributes must have shapes that makes sense, i.e., do not carry extraneous information. Thus, this needs obsgeloc and obsgeovel not to be quantities with the extraneous dimension for x,y,z, but rather cartesian representations. Hence, this builds on #5253 ~~(could be #5207 too; will rebase on master once either is merged).~~ and now that that PR is merged, this is rebased on it.

At present, I assume that any attribute will either be a "scalar" or that it has a shape that equals (or, in case of quantities, can be broadcast to) the shape of the coordinates. I.e., I don't broadcast the shape of the coordinates to those of an attribute (as some of the tests assumed would be possible). I think this makes it too complicated (and we don't do this in Time either). The one exception I make is in EarthLocation.get_itrs, where it seems to make sense that a single position would be used with multiple times.

Questions for now, especially for @taldcroft and @eteq, are if I do the checks in the initializer and in _apply sensibly.

UPDATE: now also includes SkyCoord

UPDATE 2: with #5253 in, this is rebased and should thus be easier to review...

StuartLittlefair · 2016-08-22T23:54:17Z

+                # Check shape is OK.  As a useful side product, getting the
+                # attribute also validates it.
+                value = getattr(self, fnm)
+                if self.has_data and (getattr(value, 'size', 1) > 1 and


Checking against self.shape and only checking the attributes if the frame has data leads to some issues I think. For example, it allows one to do the following:

In [5]: times.shape Out[5]: (4, 3) In [6]: altaz_frame = coord.AltAz(location=lapalma, obstime=times, ... temperature=[20,20]*u. deg_C)

That doesn't make much sense I think, but it's allowed because the frame has no data and therefore has no shape. This leads me to think that this PR should look carefully at the attributes of the BaseCoordinateFrame like shape, ndim and isscalar.

StuartLittlefair · 2016-08-23T00:30:02Z

@mhvk I don't know the coordinates package to comment much on this initial PR. Beyond the comment above I can't see much that's wrong with it. However, here's an example use case of when shape changing frames would be useful - calculating the coordinates of multiple objects at multiple obstimes in a new frame (e.g CIRS or AltAz. Something like the below works in the current 1.2 release:

In [1]: from astropy.time import Time
In [2]: from astropy import coordinates as coord, units as u
In [3]: from astropy.coordinates.tests.utils import randomly_sample_sphere
In [4]: times = Time.now() + np.linspace(0,10,12)*u.day
In [5]: lapalma = coord.EarthLocation.of_site('lapalma')
In [6]: coos = coord.SkyCoord(*randomly_sample_sphere(3))
In [7]: coos[:, np.newaxis].transform_to(coord.CIRS(obstime=times)).shape
Out[7]: (3, 12)

But with the code in this PR this fails:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-9-e86535bbb6b0> in <module>()
      5 lapalma = coord.EarthLocation.of_site('lapalma')
      6 coos = coord.SkyCoord(*randomly_sample_sphere(3))
----> 7 coos[:, np.newaxis].transform_to(coord.CIRS(obstime=times)).shape
      8 

/usr/local/lib/python3.5/site-packages/astropy/coordinates/sky_coordinate.py in transform_to(self, frame)
    401         # Do the transformation, returning a coordinate frame of the desired
    402         # final type (not generic).
--> 403         new_coord = trans(self.frame, generic_frame)
    404 
    405         # Finally make the new SkyCoord object from the `new_coord` and

/usr/local/lib/python3.5/site-packages/astropy/coordinates/transformations.py in __call__(self, fromcoord, toframe)
    913 
    914             curr_toframe = t.tosys(**frattrs)
--> 915             curr_coord = t(curr_coord, curr_toframe)
    916 
    917         # this is safe even in the case where self.transforms is empty, because

/usr/local/lib/python3.5/site-packages/astropy/coordinates/transformations.py in __call__(self, fromcoord, toframe)
    704 
    705     def __call__(self, fromcoord, toframe):
--> 706         res = self.func(fromcoord, toframe)
    707         if not isinstance(res, self.tosys):
    708             raise TypeError('the transformation function yielded {0} but '

/usr/local/lib/python3.5/site-packages/astropy/coordinates/builtin_frames/icrs_cirs_transforms.py in icrs_to_cirs(icrs_coo, cirs_frame)
     60                                          distance=srepr.distance, copy=False)
     61 
---> 62     return cirs_frame.realize_frame(newrep)
     63 
     64 

/usr/local/lib/python3.5/site-packages/astropy/coordinates/baseframe.py in realize_frame(self, representation)
    821                         for attr in self.get_frame_attr_names()
    822                         if attr not in self._attr_names_with_defaults])
--> 823         return self.__class__(representation, **frattrs)
    824 
    825     def represent_as(self, new_representation, in_frame_units=False):

/usr/local/lib/python3.5/site-packages/astropy/coordinates/baseframe.py in __init__(self, *args, **kwargs)
    663                         raise ValueError(
    664                             "frame attribute should have no shape or shape={0}"
--> 665                             ", not {1}".format(self.shape, value.shape))
    666 
    667             else:

ValueError: frame attribute should have no shape or shape=(3, 12), not (12,)

which is probably not surprising, but I'm more surprised this doesn't work either, as I'd expect the 3 objects to broadcast against the 12 times...

In [7]: coos[:, np.newaxis].transform_to(coord.CIRS(obstime=times[np.newaxis, :])).shape

ValueError: frame attribute should have no shape or shape=(3, 12), not (1, 12)

mhvk · 2016-08-23T00:37:09Z

@StuartLittlefair - good point about data-less frames. Of course, your particular example would still work if the broadcasting in the transformations makes the data have the right shape. But one would only get an error rather late if this were not the case. The one thing I could do here is to check the shapes against each other. Do you think that would be worth it?

p.s. I wondered whether it might make more sense to just disallow non-scalar attributes for data-less frames, but I see that several tests use arrays of obstime and those make a lot of sense (data gets broadcast in transformation, so the tests work).

mhvk · 2016-08-23T00:40:45Z

On your example: this could be made to work if I allowed Time to be broadcast... That would mean moving the shape-checking to the frame attributes proper, which may not be so bad... I'll try.

StuartLittlefair · 2016-08-23T00:55:47Z

@mhvk - on dataless frames: I think it would save a lot of user pain if we at least made sure that all the frame attributes are the same shape or can be broadcast.

mhvk · 2016-08-23T18:52:45Z

@StuartLittlefair - OK, that was more work than I had really anticipated, but moving the shape-checking machinery to FrameAttribute was definitely the right thing to do.

In the process:

TimeFrameAttribute can now be broadcast, with a little modification to Time._apply
CartesianRepresentationFrameAttribute: idem
Data-less frames now have the shapes of the attributes checked against each other. They also have acquired a shape.

@taldcroft - I think my mucking with BaseCoordinateFrame is all OK, but it could definitely do with a bit of checking...

mhvk · 2016-08-24T18:26:31Z

Rebased to get it mergeable again, and also added support for shape manipulation to SkyCoord itself, as it turned out to be very easy, as it has no internal attributes to worry about apart from the base frame.

Since CartesianRepresentation.__eq__ is not yet implemented.

mhvk · 2016-08-31T21:17:47Z

Another rebase now #5253 is in, which should make this PR easier to review. Note that in process of doing this, I touched Time to make it broadcastable, and moved a piece from modeling.utils to utils.misc as it was generally useful. Hence, cc @taldcroft, @nden.

Once this is in, we should add test cases for transformations of multidimensional coordinates; quite a few should "just work", but for some the transformations may need to be updated.

Also cc @eteq, @adrn, @StuartLittlefair as this is obviously a fairly involved change in coordinates. With the ability of shaped data-less frames, though, I think it is fairly complete.

mhvk · 2016-10-03T05:53:10Z

I realise this is quite a large PR, but it also brings nice functionality. A review would be good...

taldcroft · 2016-10-08T19:30:42Z

@mhvk - I'll try to have a look this week.

mhvk · 2016-10-18T16:03:54Z

@taldcroft - ping...

taldcroft

With a strong cup of coffee I got through this today. Fantastic work @mhvk.

Aside from some code comments, the only bigger picture request is to put in some narrative docs showing this. For the most part things "just work" as you might expect, but interactions with the object attributes might need explaining in general. This can be modeled on the section for Time, or perhaps factor out into a common section for both?

taldcroft · 2016-10-18T17:18:40Z

+        if self._data is None:
+            # No data: we still need to check that any non-scalar attributes
+            # have consistent shapes.
+            shapes = {fnm: value.shape for fnm, value in values.items()


It looks like this requires that any FrameAttribute in a frame has a shape attribute. I think that is OK, but then this needs to go in the base class. Or else just getattr(value, 'shape', None)?

It only requires the attribute to have a shape if it has a size (which is larger than 1). I added to the comment above to clarify this.

Ah right. The postfix conditional threw me off.

taldcroft · 2016-10-18T17:26:31Z

+                            "non-scalar attributes with inconsistent "
+                            "shapes: {0}".format(shapes))
+
+                    for fnm in shapes:


Is this validation really doing something necessary? If so then I would expect it to be checking the shape and raising an exception (basically saying there is some code bug) if not matching. Otherwise I would think that unit testing would cover this.

There are really two checks done: first, whether the shapes can be broadcast against each other; second, whether the attributes actually can be broadcast (this is true for all those that we define, but may not generally be the case). This is similar to the check done for the case that the frame does have data. (I'll again add a comment, since none of this is all that obvious...)

taldcroft · 2016-10-18T17:39:25Z

    @property
    def shape(self):
-        return self.data.shape
+        return self.data.shape if self.has_data else self._no_data_shape


This is inconsistent with the behavior of size below for the no-data case. It's also a little odd because the shape for a no-data coordinate is () which is the same as the shape for a scalar coordinate. Conceptually I think that coord.shape should give an exception (like it now does) for no-data, though I imagine there is a real reason you made this change.

Having coord.shape give an exception for no-data would be nice because you might be able to eliminate the new self._no_data_shape private attribute.

The reason I implemented it like this is that in the frame attribute I look up the frame shape, and I felt the attribute should not have to worry about whether that frame has data or not.

More generally, it also seemed there should be a way to tell what shape the output would have if you transformed, say, a single coordinate to a frame with multiple times (say, an AltAz frame). With that, it is also clearer what to expect when you reshape or ravel a data-less frame.

But I do agree it is slightly counterintuitive to get an AttributeError when you access size and not when you access shape. Clearly, this is something that I should note explicitly in the to-be-written documentation!

taldcroft · 2016-10-18T17:46:47Z

@@ -120,25 +121,33 @@ def get_name(cls):
    def _apply(self, method, *args, **kwargs):
        """Create a new coordinate object with ``method`` applied to the arrays.


new representation object, right?

taldcroft · 2016-10-18T17:54:18Z

-            *[getattr(getattr(self, component), method)(*args, **kwargs)
-              for component in self.components], copy=False)
+        if callable(method):
+            apply_method = functools.partial(method, *args, **kwargs)


It seems like this won't actually work if any *args are supplied because the later call of apply_method(array, ...) will put array after the *args according to the functools.partial docs.

In [11]: c.data._apply('reshape', (2,2)) Out[11]: <UnitSphericalRepresentation (lon, lat) in deg [[(1.0, 1.0), (2.0, 2.0)], [(3.0, 3.0), (4.0, 4.0)]]> In [12]: c.data._apply(np.reshape, (2,2)) ... TypeError: Only dimensionless scalar quantities can be converted to Python scalars In [13]: c.data._apply(np.ravel) Out[13]: <UnitSphericalRepresentation (lon, lat) in deg [(1.0, 1.0), (2.0, 2.0), (3.0, 3.0), (4.0, 4.0)]>

OOPS!! Clearly, bad luck that I wrote all my tests using keyword arguments only... I'll just define the function directly...

taldcroft · 2016-10-18T18:39:10Z

+        assert np.all(s0_broadcast.lon == self.s0.lon)
+        assert np.all(s0_broadcast.lat == self.s0.lat)
+        assert np.all(s0_broadcast.distance == self.s0.distance)
+        assert np.may_share_memory(s0_broadcast.lon, self.s0.lon)


Maybe at least one test that the do share memory?

Yes, good idea. Also noticed that I didn't have a default copy method in SharedLikeNDArray. That is now corrected as well.

taldcroft · 2016-10-18T18:41:26Z

        assert np.all(s0_take.lon == self.s0.lon.take((5, 2)))
+
+    def test_broadcast_to(self):
+        s0_broadcast = self.s0._apply(np_broadcast_to, shape=(3, 6, 7),


Test that the class is the same.

Yes, good idea. Done.

taldcroft · 2016-10-18T18:47:27Z

+                                       self.s2.obstime.jd1)
+        # TODO: right now, we cannot compare CartesianRepresentation instances,
+        # so instead we compare the xyz quantities.  This can be made a direct
+        # comparison once CartesianRepresentation.__eq__ is implemented.


Update comment per decision to not support __eq__.

taldcroft · 2016-10-18T18:54:43Z

 from ..extern.six.moves import range, zip_longest, zip

-from ..utils import isiterable
+from ..utils import isiterable, check_broadcast


Import IncompatibleShapeError as well for back-compatibility with previously available symbol in this module.

In the original version, IncompatibleShapeError was not exported, so I think it is OK not to keep it here, no?

I am not always sure about drawing the line in what is "public", but there could be code like from astropy.modeling.utils import IncompatibleShapeError out there, especially for something that uses check_broadcast and then wants to catch the error. Agreed that none of this is actually in the public API so I don't feel too strongly.

taldcroft · 2016-10-18T18:58:29Z

            new_format = self.format

+        if callable(method):
+            apply_method = functools.partial(method, *args, **kwargs)


Same question about whether this works if *args are supplied.

mhvk · 2016-10-20T15:18:11Z

@taldcroft - I now also made some additions to the documentation; see in particular 4b21463 (which should be OK apart from the misspelled link that is corrected in the last commit). I think with this, it should be ready to go in...

taldcroft · 2016-10-21T20:42:50Z

OK 👍 for merge. @mhvk - I'll let you press the button in case you want to merge #5301 first (if that matters).

mhvk · 2016-10-21T20:54:09Z

Thanks! Order doesn't really matter for this (I probably need to rebase either way...), so I'll go ahead here!

mhvk added coordinates Enhancement Work in progress labels Aug 22, 2016

mhvk added this to the v1.3.0 milestone Aug 22, 2016

StuartLittlefair reviewed Aug 22, 2016
View reviewed changes

mhvk force-pushed the baseframe-shape-changes branch from 9c82e3c to 2d65574 Compare August 23, 2016 21:30

mhvk removed the Work in progress label Aug 23, 2016

mhvk mentioned this pull request Aug 24, 2016

Finishing up CartesianRepresentationFrameAttribute #5253

Merged

mhvk changed the title ~~Baseframe shape changes~~ SkyCoord/Baseframe shape changes Aug 24, 2016

mhvk force-pushed the baseframe-shape-changes branch 2 times, most recently from bf54ac8 to 70983f6 Compare August 24, 2016 18:07

mhvk force-pushed the baseframe-shape-changes branch from 70983f6 to 59d9f7e Compare August 24, 2016 19:27

mhvk added 9 commits August 31, 2016 15:59

Give BaseFrame the ShapedLikeNDArray mixin.

20ac608

Ensure QuantityFrameAttribute doesn't convert data needlessly.

6229290

Ensure EarthLocation broadcasts input correctly.

3f579aa

Consistency check of frame data and frame attributes.

2297189

Ensure tests don't assume scalar pos & non-scalar time work.

e3d3e6a

Tests for frame reshape, etc.

e666656

Move shape consistency check to FrameAttribute.

69d62d3

Simplify Time._apply.

db1971f

Allow Time._apply to take callable methods and test broadcasting.

301682a

mhvk added 2 commits August 31, 2016 16:01

Changelog update.

c37688f

Ensure we compare CartesianRepresentations using their Quantity form.

4c3dafd

Since CartesianRepresentation.__eq__ is not yet implemented.

mhvk force-pushed the baseframe-shape-changes branch from 59d9f7e to f2e0af9 Compare August 31, 2016 21:11

Redo broadcasting, now all FrameAttributes are broadcastable.

b917bb2

mhvk force-pushed the baseframe-shape-changes branch from f2e0af9 to b917bb2 Compare September 1, 2016 23:31

mhvk mentioned this pull request Sep 28, 2016

AltAz -> Galactic, with multiple observation times #5364

Closed

mhvk mentioned this pull request Oct 13, 2016

Reshaping SkyCoord #5404

Closed

taldcroft reviewed Oct 18, 2016

View reviewed changes

taldcroft self-assigned this Oct 18, 2016

mhvk added 3 commits October 19, 2016 21:45

Changes/improvements following comments from Tom Aldcroft.

2837ef9

Add documentation about shaping of coordinates, representations, frames.

4b21463

Fix documentation link.

de1aea7

mhvk added the Ready-for-final-review label Oct 20, 2016

mhvk added Affects-dev PRs and issues that do not impact an existing Astropy release and removed Ready-for-final-review labels Oct 21, 2016

mhvk merged commit c8fd6ec into astropy:master Oct 21, 2016

mhvk deleted the baseframe-shape-changes branch October 21, 2016 20:54

This was referenced Oct 21, 2016

Representation arithmetic #5301

Merged

Numpy dev array printing - update all tests to new numpy style #5423

Merged

StuartLittlefair mentioned this pull request Nov 17, 2016

get_moon does not support non-scalar times for non-geocentric observer #5216

Closed

bsipocz mentioned this pull request Dec 21, 2016

Stop recognizing scalar SkyCoord objects as vectors astropy/astroplan#274

Merged

bsipocz mentioned this pull request Dec 29, 2016

Unable to disable remote_data test option #5651

Closed

mhvk mentioned this pull request Aug 18, 2023

Broadcast frames over both data and attributes #15121

Merged

		@@ -120,25 +121,33 @@ def get_name(cls):
		def _apply(self, method, args, *kwargs):
		"""Create a new coordinate object with ``method`` applied to the arrays.

Conversation

mhvk commented Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StuartLittlefair Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StuartLittlefair commented Aug 23, 2016

Uh oh!

mhvk commented Aug 23, 2016

Uh oh!

mhvk commented Aug 23, 2016

Uh oh!

StuartLittlefair commented Aug 23, 2016

Uh oh!

mhvk commented Aug 23, 2016

Uh oh!

mhvk commented Aug 24, 2016

Uh oh!

mhvk commented Aug 31, 2016

Uh oh!

mhvk commented Oct 3, 2016

Uh oh!

taldcroft commented Oct 8, 2016

Uh oh!

mhvk commented Oct 18, 2016

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Oct 20, 2016

Uh oh!

taldcroft commented Oct 21, 2016

Uh oh!

mhvk commented Oct 21, 2016

Uh oh!

Reviewers

mhvk commented Aug 22, 2016 •

edited

Loading

StuartLittlefair Aug 22, 2016 •

edited

Loading