Implement AffineTransforms by adrn · Pull Request #10 · adrn/astropy

adrn · 2017-06-10T01:19:48Z

No description provided.

eteq · 2017-06-12T04:18:41Z

-class StaticMatrixTransform(CoordinateTransform):
+        return newrep
+
+class AffineTransform(BaseAffineTransform):


This (and probably BaseAffineTransform) should be in __all__ , right? That is, they shold be considered public API?

mhvk

This looks very good, and my comments are mostly minor, but for larger ones, see astropy#6218 (comment) (so discussion stays on the astropy github site).

mhvk · 2017-06-15T22:23:44Z

-    rotation = rotation_matrix(-np.arcsin(z_d), 'y')
-    representation = (intrep - translation).transform(rotation)
+    translation = CartesianRepresentation(gcf.galcen_distance * [1., 0., 0.])
+    z_d = (gcf.z_sun / gcf.galcen_distance).decompose()


Why the decompose? I'd think the arcsin would (effectively) do that for you.

Yea, not sure - that was in there before, but will update now since I'm touching this code.

mhvk · 2017-06-15T22:27:55Z

-    bary_sun_pos = get_body_barycentric('sun', hcrs_coo.obstime)
-    newrep = hcrs_coo.cartesian + bary_sun_pos
-    return icrs_frame.realize_frame(newrep)
+    bary_sun_vel = None


This seems unused.

It's used on L298, a few lines below this.

Oh, I see what you mean now :)

mhvk · 2017-06-15T22:29:04Z

+@frame_transform_graph.transform(AffineTransform, ICRS, HCRS)
 def icrs_to_hcrs(icrs_coo, hcrs_frame):
    # this is just an origin translation so without a distance it cannot go ahead
    if icrs_coo.data.__class__ == UnitSphericalRepresentation:


While you are here, replace == with is (or do isinstance(icrs_coo.data, UnitSphericalRepresentation))

mhvk · 2017-06-15T22:29:59Z

    # already got unregistered when trans2 was created.
    trans2.unregister(frame_transform_graph)

-


Two empty lines between functions is actually pep8...

mhvk · 2017-06-15T22:33:54Z

+    """
+
+    def _apply_transform(self, fromcoord, matrix, vectors):
+        from .representation import (CartesianRepresentation,


Why not import these at the top?

Ran in to circular import issues, but I'll check again to confirm.

mhvk · 2017-06-15T22:42:40Z

+                newdiff = newdiff.transform(matrix)
+
+            if vectors is not None and vectors[1] is not None:
+                vel_offset = CartesianRepresentation(vectors[1])


Same as above, vectors[1] should probably be CartesianRepresentation. Or, perhaps better, just let it be an associated CartesianDifferential?

mhvk · 2017-06-15T22:49:08Z

+    subclasses.
+    """
+
+    def _apply_transform(self, fromcoord, matrix, vectors):


Move to CartesianRepresentation.transform? (since this may be discussion to keep, see larger comment in the astropy thread: astropy#6218 (comment))

mhvk · 2017-06-15T22:51:02Z

+            newdiff = newdiff.represent_as(CartesianDifferential)
+            newrep._differentials = (newdiff,)
+
+        if issubclass(fromcoord.data.__class__, UnitSphericalRepresentation):


Just isinstance(fromcoord.data, UnitSphericalRepresentation)?

Sure - I think this was also in the code already, but will change.

mhvk · 2017-06-15T22:52:54Z

+                vel_offset = CartesianRepresentation(vectors[1])
+                newdiff = newdiff + vel_offset
+
+            newdiff = newdiff.represent_as(CartesianDifferential)


I think here it would be better to represent it back as the type it was before, with newrep as a base.

mhvk · 2017-06-15T23:01:40Z

+    def __call__(self, fromcoord, toframe):
+
+        M, vec = self.transform_func(fromcoord, toframe)
+        newrep = self._apply_transform(fromcoord, M, vec)


So this would become

newrep = fromcoord.represent_as(CartesianRepresentation).transform(M, vec)

…ocity offset vector

…ials and assume transform funcs return representations

…sets

…n and all differentials to cartesian form

…pty dict is passed

adrn · 2017-06-18T15:30:39Z

        # Note: the above docstring gets overridden for differentials.
        raise NotImplementedError()

+    def to_cartesian_with_differentials(self):


@mhvk @eteq I could use this for brevity in the affine transform, so this is one possibility for a short-hand

Am still not too keen to add this to representation, as I don't like the duplication of to_cartesian and find the name rather long (but don't have a better suggestion).

How about using it for .cartesian on a frame, though? For that case, I feel there is no doubt one wants to transform the derivatives as well, and we know that we only have first-order to worry about. I guess that means adapting represent_as. We can always decide to move it down to representation later.

adrn · 2017-06-18T15:31:13Z

            return dict()

-        if not self.differentials:
+        if not self.differentials and differential_class:


Only fail if an empty dict is passed and there are no differentials - this allows for slightly cleaner code elsewhere.

This is a good idea.

adrn force-pushed the coordinates/affine-transforms branch from 8593562 to 9ccff22 Compare June 10, 2017 17:45

adrn force-pushed the coordinates/diffs-on-reps branch from f0b88f3 to bc08ff6 Compare June 11, 2017 20:06

adrn force-pushed the coordinates/affine-transforms branch from 72386ac to c05c77f Compare June 11, 2017 20:08

eteq reviewed Jun 12, 2017

View reviewed changes

adrn force-pushed the coordinates/affine-transforms branch from 6f68461 to d74bd6a Compare June 12, 2017 16:16

adrn changed the title ~~Implement AffineTransforms and a proof of concept ICRS-Galactocentric demo~~ Implement AffineTransforms Jun 12, 2017

adrn force-pushed the coordinates/diffs-on-reps branch from bc08ff6 to 2725e80 Compare June 13, 2017 16:18

adrn force-pushed the coordinates/affine-transforms branch 2 times, most recently from 16dce30 to 0960e2b Compare June 14, 2017 01:17

adrn force-pushed the coordinates/diffs-on-reps branch from f08e128 to 03f04dd Compare June 14, 2017 20:30

adrn force-pushed the coordinates/affine-transforms branch from 04c773b to db13a87 Compare June 14, 2017 20:31

adrn mentioned this pull request Jun 15, 2017

Add AffineTransform transform class astropy/astropy#6218

Merged

mhvk reviewed Jun 15, 2017

View reviewed changes

adrn force-pushed the coordinates/affine-transforms branch from db13a87 to 3abf575 Compare June 16, 2017 01:55

adrn added 16 commits June 18, 2017 10:13

added an AffineTransform class, but things are broken (merge conflict)

42cf752

working state

24b0e5c

add velocity support to apply_transform

cbc90b4

switch galactocentric frame to use affinetransform, and to return vel…

fafb7a6

…ocity offset vector

turn differential back into a ... differential

de0c991

make AffineTransforms picklable by reworking slightly

f037d78

no-op if vector offset is None

9900456

ignore matrix transform is matrix is None

c34d400

remove attribute and ignore velocity offset for now

68ba5c2

add affine transform classes to __all__

b980d66

oops also remove unneeded import

1e66a96

fix transform when differentials and no matrix

e88368d

refine and refactor tests

351786e

fix order of operations bug in affine transform

2a7d8a5

ICRS to HCRS transformation is an AffineTransform

a6dc8dc

add section for affinetransform

5cf3d46

adrn added 5 commits June 18, 2017 10:13

address minor comments from mhvk

465cde4

allow BaseRepresentation.transform() to operate on attached different…

a509d03

…ials and assume transform funcs return representations

return representations as offsets with differentials for velocity off…

60322f9

…sets

update tests to use new transforms

bdd36a0

re-represent transformed differential to original class

df5081f

adrn force-pushed the coordinates/affine-transforms branch from 4de3f11 to df5081f Compare June 18, 2017 14:17

adrn added 4 commits June 18, 2017 10:28

use new transform function that operates on differentials by default

713f907

added to_cartesian_with_differentials method to convert representatio…

b07a0d4

…n and all differentials to cartesian form

allow represent_as to succeed if there are no differentials and an em…

0dcf392

…pty dict is passed

tidied up and commented affine transform core

5a93662

adrn commented Jun 18, 2017

View reviewed changes

adrn closed this Jun 18, 2017

		# already got unregistered when trans2 was created.
		trans2.unregister(frame_transform_graph)

Conversation

adrn commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants