optimize representation shortcuts when have differentials by nstarman · Pull Request #11467 · astropy/astropy

nstarman · 2021-04-02T17:48:39Z

Optimize representation shortcuts when there are attached differentials.
In particular, this helps avoid NaN leakage from the distance to angular components.
Applied suggested . _re_represent_differentials() method.

Split from #11444 (suggested in comment by @mhvk)

TODO:

changelog
tests

Signed-off-by: Nathaniel Starkman (@nstarman) [email protected]

pep8speaks · 2021-04-02T17:48:47Z

Hello @nstarman 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-04-11 21:09:48 UTC

mhvk · 2021-04-02T19:31:10Z

Yes, good idea! And of course tricky that the tests actually fail. I fear UnitSpherical may need special treatment, again... When we are done with getting transformations right, I wonder if we can get rid of it altogether by just using distance=nan for all cases where it is now used! Anyway, for later!

nstarman · 2021-04-03T07:01:47Z

@mhvk, I commented out the UnitSpherical case, which requires #11469. All other cases appear to work.

mhvk · 2021-04-03T17:01:09Z

#11469 is in, so perhaps good to rebase and check that unitspherical works too?

nstarman · 2021-04-03T20:39:51Z

@mhvk @adrn . Rebased, but it doesn't appear to fix the problem. The issue is that re-representing unit differentials as full differentials doesn't seem to be resolved in this case. The unit keys in the differentials dict don't match the units on the representation, I think because the representation is given a distance=1.0 without units.

The first error I'm getting is:

ValueError: For differential object '<SphericalDifferential (d_lon, d_lat, d_distance) in (km rad / s, km rad / s, km / s)
E                       [(0., 0.,  1.), (0., 0., 10.)]>', expected unit key = 's / m' but received key = 's

full error: error.log

Edit: also, any recommendations where the tests should go? there are a couple representation test files.

mhvk · 2021-04-03T20:51:19Z

That error indeed suggests the problem is similar (the norm is removed before one has a chance to divide the angular differentials by it), but apparently #11469 only fixed one route. I'll try to have a look later.

As for tests, I think this belongs to basic operations, so test_representation.py.

mhvk · 2021-04-04T17:19:08Z

            if issubclass(other_class, PhysicsSphericalRepresentation):
-                return other_class(phi=self.lon, theta=90 * u.deg - self.lat, r=1.0,
-                                   copy=False)
+                # diffs = self._re_represent_differentials(other_class,


I think part of the reason this fails is that baseframe asks for something impossible - to always transform to SphericalDifferential. Probably best to raise a separate issue...

So can we merge before this is resolved, and then followup PR to uncomment those lines, or just wait?

I suspect #11467 (comment) will not be trivial. Perhaps the best route is to not use _re_represent_differentials for this function as it preserves the unit keys.

Not quite sure how much it really helps, but FWIW, tests seem to pass if I write the if-statement as:

elif (issubclass(other_class, SphericalRepresentation) and (not differential_class or all(diff.base_representation is self.__class__ for diff in self.differentials.values()))):

That at least is closer to what we want...

Interesting 🤔 ... Obviously it's up to you and the maintainers, but my personal preference would be not not complicate the if-blocks, merge in this PR without optimizing the UnitSpherical case, and then tackle the problem after that... But happy any which way 👍 .

I agree my suggestion is not great either, so I'm OK with simply not addressing UnitSpherical here. But in that case I think we should remove the commented out code - since that does not work right now, I don't think it is helpful. A more detailed comment about why it is tricky (maybe just referring to this discussion) would also be good.

I just removed the commented out code in representations.py, but left it in test_representations.py as that code can just be uncommented as soon as we find a UnitsSpherical fix.

nstarman · 2021-04-05T00:16:07Z

what happened to codecov?

mhvk

Quick comment on the tests, which I think should check that the fast route is actually taken. I've got a fix for #11477, so perhaps we can wait for that? On the other hand, am also happy to just get it in...

mhvk · 2021-04-06T01:33:19Z

#11482 is in, so rebased on master, UnitSpherical should now work too...

nstarman · 2021-04-06T01:48:58Z

#11482 is in, so rebased on master, UnitSpherical should now work too...

~~Done! @mhvk~~
~~Oh weird! it passed my tests.~~
I was not in my development environment.

There's still a problem.

def test_setitem_velocities():
        """Test different flavors of item setting for a Frame with a velocity.
    
        """
        sc0 = FK4([1, 2]*u.deg, [3, 4]*u.deg, radial_velocity=[1, 2]*u.km/u.s,
                  obstime='B1950')
        sc2 = FK4([10, 20]*u.deg, [30, 40]*u.deg, radial_velocity=[10, 20]*u.km/u.s,
                  obstime='B1950')
    
        sc1 = sc0.copy()
        sc1[1] = sc2[0]
>       assert np.allclose(sc1.ra.to_value(u.deg), [1, 10])

astropy/coordinates/tests/test_frames.py:600: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
astropy/coordinates/baseframe.py:1642: in __getattr__
    rep = self.represent_as(self.representation_type,
astropy/coordinates/baseframe.py:1099: in represent_as
    data = self.data.represent_as(representation_cls,
astropy/coordinates/representation.py:1592: in represent_as
    return other_class(lon=self.lon, lat=self.lat, distance=1.0,
astropy/coordinates/representation.py:1808: in __init__
    super().__init__(lon, lat, distance, copy=copy,
astropy/coordinates/representation.py:664: in __init__
    self._differentials = self._validate_differentials(differentials)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <SphericalRepresentation (lon, lat, distance) in (deg, deg, )
    [( 1.,  3., 1.), (10., 30., 1.)]>, differentials = {'s': <RadialDifferential (d_distance) in km / s
    [( 1.,), (10.,)]>}

    def _validate_differentials(self, differentials):
        """
        Validate that the provided differentials are appropriate for this
        representation and recast/reshape as necessary and then return.
    
        Note that this does *not* set the differentials on
        ``self._differentials``, but rather leaves that for the caller.
        """
    
        # Now handle the actual validation of any specified differential classes
        if differentials is None:
            differentials = dict()
    
        elif isinstance(differentials, BaseDifferential):
            # We can't handle auto-determining the key for this combo
            if (isinstance(differentials, RadialDifferential) and
                    isinstance(self, UnitSphericalRepresentation)):
                raise ValueError("To attach a RadialDifferential to a "
                                 "UnitSphericalRepresentation, you must supply "
                                 "a dictionary with an appropriate key.")
    
            key = differentials._get_deriv_key(self)
            differentials = {key: differentials}
    
        for key in differentials:
            try:
                diff = differentials[key]
            except TypeError:
                raise TypeError("'differentials' argument must be a "
                                "dictionary-like object")
    
            diff._check_base(self)
    
            if (isinstance(diff, RadialDifferential) and
                    isinstance(self, UnitSphericalRepresentation)):
                # We trust the passing of a key for a RadialDifferential
                # attached to a UnitSphericalRepresentation because it will not
                # have a paired component name (UnitSphericalRepresentation has
                # no .distance) to automatically determine the expected key
                pass
    
            else:
                expected_key = diff._get_deriv_key(self)
                if key != expected_key:
>                   raise ValueError("For differential object '{}', expected "
                                     "unit key = '{}' but received key = '{}'"
                                     .format(repr(diff), expected_key, key))
E                   ValueError: For differential object '<RadialDifferential (d_distance) in km / s
E                       [( 1.,), (10.,)]>', expected unit key = 's / m' but received key = 's'

astropy/coordinates/representation.py:710: ValueError

When FK4 gets initialized, I ~~think~~ know radial_velocity has unit key "s", even though the FK4 data has unit-spherical data, so the unit key should(?) be "s/m".

Here is the step from this PR.

astropy/coordinates/representation.py:1592: in represent_as
return other_class(lon=self.lon, lat=self.lat, distance=1.0,

nstarman · 2021-04-06T02:41:21Z

I think the issue stems from / is strongly related to

astropy/astropy/coordinates/baseframe.py

Line 556 in 02e54d3

    
           representation_data = representation_data.with_differentials({'s': differential_data})

Since for some situations it should be "s/m"

nstarman · 2021-04-06T03:02:35Z

There's also an unexpected lack of problem for UnitSpherical to PhysicsSpherical. I imagine the same error should be encountered, but no test in baseframe.py covers that scenario.

pllim · 2021-04-06T13:59:49Z

I need to test the bot and unfortunately the bot hates all my PRs, so I am going to close this and then re-open it after a few seconds to see if the towncrier check kicks off, given you have a fragment file already. Sorry for any inconvenience caused and thank you for your understanding!

Update: Something isn't right in the bot... Will follow up on OpenAstronomy/baldrick#111

mhvk · 2021-04-06T19:46:17Z

Weird, I thought I had checked this and that tests passed. Must have messed up....

@nstarman

applied suggested ``. _re_represent_differentials()`` method. tests Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

mhvk · 2021-04-11T21:02:23Z


+    def test_representation_shortcuts(self):
+        """Test that shortcuts in ``represent_as`` don't fail."""
+        # diffs = UnitSphericalCosLatDifferential(4*u.mas/u.yr, 5*u.mas/u.yr,


Sorry to be nitpicky, but could you add a comment here too? Nothing worse than commented-out code without a reason!
Fine to add a commit with [ci skip] in the commit message to avoid all the tests running again...

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

mhvk

OK, let's get this in! Thanks, @nstarman!

github-actions Bot added the coordinates label Apr 2, 2021

nstarman marked this pull request as draft April 2, 2021 18:10

mhvk mentioned this pull request Apr 2, 2021

Fix conversion of cartesian to unit-spherical coordinates with velocities #11469

Merged

nstarman force-pushed the shortcut-represent_as-with-differentials branch from cc84b76 to 7a6db14 Compare April 3, 2021 06:44

nstarman marked this pull request as ready for review April 3, 2021 07:32

adrn reviewed Apr 3, 2021

View reviewed changes

Comment thread astropy/coordinates/representation.py Outdated

nstarman force-pushed the shortcut-represent_as-with-differentials branch from 1f4cedc to 81ab7ff Compare April 3, 2021 20:31

mhvk reviewed Apr 4, 2021

View reviewed changes

mhvk mentioned this pull request Apr 4, 2021

SkyCoord/BaseFrame.represent_as does not treat differentials very logically #11477

Closed

nstarman force-pushed the shortcut-represent_as-with-differentials branch 2 times, most recently from fe0cfcd to 6dee24f Compare April 4, 2021 19:13

nstarman requested review from adrn and mhvk April 4, 2021 22:39

mhvk reviewed Apr 5, 2021

View reviewed changes

Comment thread astropy/coordinates/tests/test_representation.py

nstarman force-pushed the shortcut-represent_as-with-differentials branch from 6dee24f to 273b52e Compare April 5, 2021 04:59

nstarman force-pushed the shortcut-represent_as-with-differentials branch from 273b52e to 43fb244 Compare April 6, 2021 01:47

nstarman force-pushed the shortcut-represent_as-with-differentials branch from 43fb244 to e0da930 Compare April 6, 2021 02:55

pllim closed this Apr 6, 2021

pllim reopened this Apr 6, 2021

pllim added this to the v4.3 milestone Apr 6, 2021

optimize representation shortcuts when differentials

f3fda96

applied suggested ``. _re_represent_differentials()`` method. tests Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

nstarman force-pushed the shortcut-represent_as-with-differentials branch from e0da930 to f3fda96 Compare April 11, 2021 20:51

mhvk reviewed Apr 11, 2021

View reviewed changes

comment on unimplementable test [ci skip]

9ebbea2

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>

mhvk approved these changes Apr 11, 2021

View reviewed changes

mhvk merged commit 267938b into astropy:main Apr 11, 2021

nstarman deleted the shortcut-represent_as-with-differentials branch April 11, 2021 21:20

Conversation

nstarman commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-04-11 21:09:48 UTC

Uh oh!

mhvk commented Apr 2, 2021

Uh oh!

nstarman commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mhvk commented Apr 3, 2021

Uh oh!

nstarman commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Apr 3, 2021

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!

nstarman commented Apr 5, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk commented Apr 6, 2021

Uh oh!

nstarman commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Apr 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nstarman commented Apr 2, 2021 •

edited

Loading

pep8speaks commented Apr 2, 2021 •

edited

Loading

nstarman commented Apr 3, 2021 •

edited

Loading

nstarman commented Apr 3, 2021 •

edited

Loading

nstarman commented Apr 6, 2021 •

edited

Loading

nstarman commented Apr 6, 2021 •

edited

Loading

nstarman commented Apr 6, 2021 •

edited

Loading

pllim commented Apr 6, 2021 •

edited

Loading