Remove use of np.matrix in coordinates, version 2 by mhvk · Pull Request #5104 · astropy/astropy

mhvk · 2016-06-15T17:18:23Z

This is the second of two possible ways to remove the use of np.matrix in coordinates (see #5088).

In this PR, the matrices are replaced with normal arrays, and a new matmul function is defined that either just uses the numpy function of that name, or, for numpy <= 1.10, the equivalent einsum. With this implementation, in all the transformations, multiplication has to be replaced with matmul, so this affects a lot more files. To get general broadcasting to work, it will be necessary to also replace any .T on the rotation matrices with .swapaxes(-2, -1).

mhvk · 2016-06-15T17:32:38Z

p.s. If we go this route, matmul should probably be defined in utils.compat.numpy rather than in angles, but this can obviously be done later.

astrofrog · 2016-06-16T14:16:59Z

    mat3 = rotation_matrix(Galactic._ngp_J2000.ra.degree, 'z')

-    return mat1 * mat2 * mat3 * pmat
+    return matmul(matmul(matmul(mat1, mat2), mat3), pmat)


I wonder if we should have a standard way of indicating 'review this when we only support python 3.5 and higher', or 'review this when we only support numpy 1.11 or higher', etc.?

What about having a very distinct color and naming for a label? Something like "revise after: np 1.11"?

I think this should be something in the code though, not just labels. But yes, a syntax like:

# REVISE PY35: ...

could work

It indeed needs to be something within a comment, since one cannot have @ as part of the code, even if protected by an if statement.

I do think, though, that the code will look clean enough if I define matrix_product (which repeatedly multiplies matrices together) and perhaps also a matrix_transpose.

You could wrap it in an if py35: eval("a @ b") but granted that's not exactly good style.

But the @ operator is really bad for backwards-compatibility because implementing it means that older python are not just not supported anymore but it will be impossible to use them with astropy because of the SyntaxErrors. So maybe using eval() until 2020 (?) would be the best way if astropy doesn't want to exclude any older python version.

mhvk · 2016-06-16T19:39:20Z

Since the move to matmul seemed to be preferred, I went ahead and simplified expressions here by using matrix_product (which multiplies its arguments in sequence using reduce). I also made a matrix_transpose to have at least a hope multidimensional matrices will work (though I haven't added any test cases yet; PRs to my PR more than welcome...).

p.s. The only place where I did not yet remove .T is in ecliptic_transforms, since for that I feel #4293 should be resolved first.

astrofrog · 2016-06-16T20:20:19Z

@mhvk - have you done any kind of performance benchmarking to see how things change with this PR?

mhvk · 2016-06-16T20:32:49Z

Not beyond checking that np.einsum(...) is faster than multiplying to np.matrix instances. I'll need to remind myself how to do asv...

mhvk · 2016-06-22T14:52:28Z

@eteq, @StuartLittlefair - would you be able to review this PR to remove the use of np.matrix? It passes all tests, and I think it is an improvement in itself. Next up would be to add full tests for broadcasting of coordinates and times, etc., but my own sense is that that is better done as a separate PR, since this one is already quite large.

StuartLittlefair · 2016-06-24T14:41:02Z

+        Either ``'x'``, ``'y'``, ``'z'``, or a (x,y,z) specifying the axis to
+        rotate about. If ``'x'``, ``'y'``, or ``'z'``, the rotation sense is
+        counterclockwise looking down the + axis (e.g. positive rotations obey
+        left-hand-rule).  If given as an array, the last dimension should be 3.


Is it a good idea to also allow an array of strs? For example ['x', 'y', 'z']?

'x', 'y', and 'z' are mostly meant as short-cuts to speed up those common cases; the speed-up would certainly be lost for multiple values, and since this is used mostly internally anyway, I'd tend to leave it alone.

StuartLittlefair · 2016-06-24T15:04:04Z

@mhvk this is a 👍 from me.

mhvk · 2016-06-27T20:46:59Z

@eteq - do you want to review this, or are you happy with @StuartLittlefair's review?

@astrofrog - on possible speed regressions: I think this should only lead to improvements, but still haven't figured out how to easily run asv. However, a quick test seems promising:

from astropy.coordinates import SkyCoord, Galactic
import astropy.units as u
sc = SkyCoord(np.arange(100.)*u.hourangle, np.zeros(100)*u.deg)
%timeit sc.transform_to(Galactic)
# Debian [1.1.2]: 10 loops, best of 3: 16.5 ms per loop
# This PR:       100 loops, best of 3: 14.3 ms per loop

mhvk · 2016-07-04T16:48:06Z

@eteq - with @StuartLittlefair's review, I'm confident enough to merge this removal of the use of np.matrix in coordinates, but it might still be good if you could have a look as well. If not, one question: do you think this needs an entry in the API change section for 1.3? Obviously, it only changes internal routines, so strictly it is not necessary, but it might still be good to warn that rotation_matrix now returns ndarray just in case anybody has been using the routine for their own coordinate system transforms.

eteq · 2016-07-05T15:19:45Z

I definitely plan to have a look at this, @mhvk. It might take me a few more days but I'll try to bump it up in the queue seeing as how you think it's pretty much there.

One possible logistical complication: I think I've worked out a fix to #5133 that would make be made easier by use of this machinery. But ideally we would fix that in 1.2.x and this PR is certainly 1.3.x. I might just try for a "quick-and-dirty" fix that would work in 1.2.x, and then a better version to merge after this, but in that case we'd want to get the #5133 fix in before this.

mhvk · 2016-07-05T15:24:57Z

@eteq - sounds good. I'm off on holidays at the end of the week, but happy to rebase beforehand with the fix for #5133 beforehand.

StuartLittlefair · 2016-07-05T18:29:49Z

I've just discovered the transform method of CartesianRepresentation.

Given the changes here, is this PR a sensible place to update that method to use the code contained within this PR?

and use it in latter.

… any more.

…ties.

mhvk · 2016-07-27T14:49:58Z

@eteq, @adrn - could you check you're happy with the deprecation warning, and with how I moved routines to a new matrix_utilities file? I'd like to merge this, as it is holding up further work on shape changes, etc.

adrn · 2016-07-27T15:04:58Z

This looks good to me -- thanks @mhvk !

eteq · 2016-07-29T19:38:19Z

Looks good to me too except for one super minor thing: I think this should go in the "other" section of the changelog, as it isn't really a bug fix (but isn't exactly an API change due to it not explicitly being public API). Will just do that in a manual merge though so as not to delay things further.

Thanks for all the work on this, @mhvk!

eteq · 2016-07-29T19:47:40Z

and just for the record, if anyone else is looking at this and wondering, the travis failures were a separate problem, reported in #5198 and fixed in #5203

Remove use of np.matrix in coordinates, version 2

eteq · 2016-07-29T20:01:21Z

@mhvk - didn't think this needed delaying the merge, but I see you modified the Sgr example to use the new matrices but left in this:

SGR_MATRIX = np.array(B.dot(C).dot(D))

That could be changed to

SGR_MATRIX = matrix_product(B, C, D)

right?

mhvk · 2016-07-29T20:46:25Z

Yes, sorry I missed that... Will look at #5210 tomorrow.

mhvk added coordinates Enhancement Work in progress labels Jun 15, 2016

mhvk added this to the v1.3.0 milestone Jun 15, 2016

astrofrog reviewed Jun 16, 2016
View reviewed changes

mhvk force-pushed the coord-no-np-matrix-use-matmul branch from b8ace7d to 36e0ad2 Compare June 16, 2016 19:54

mhvk force-pushed the coord-no-np-matrix-use-matmul branch from 36e0ad2 to c733163 Compare June 16, 2016 20:29

StuartLittlefair reviewed Jun 24, 2016
View reviewed changes

mhvk force-pushed the coord-no-np-matrix-use-matmul branch 2 times, most recently from c3404fd to 1068c0c Compare June 27, 2016 20:14

mhvk added Ready-for-final-review Refactoring Enhancement and removed Enhancement Work in progress labels Jun 27, 2016

mhvk mentioned this pull request Jun 28, 2016

Remove use of np.matrix in coordinates, version 1 #5103

Closed

mhvk added 14 commits July 27, 2016 16:47

Add matmul to utils.compat.numpy.

67f85b3

Ensure numpy compat tests actually get run.

a20d1d0

Define matrix_product and matrix_transpose utilities.

1a47dca

Use matrix_product and matrix_tranpose in coordinate transformations.

be536da

Simplify cartrepr_from_matmul.

e4918f3

Update docstring of cartrepr_from_matmul.

d9e916a

matmul einsum replacement correction for numpy < 1.10.

e38429f

Ensure no tests are run in stride_tricks

a9ff749

Adapt Cartesian.transform following cartrepr_from_matmul

05425ab

and use it in latter.

Remove cartrepr_from_matmul altogether, since it hardly does anything…

56e4c04

… any more.

Explain rationale of matrix_product and matrix_transpose in docstrings.

848d692

Deprecate angles.rotation_matrix/angle_axis in favour of matrix_utili…

6c06dfc

…ties.

Move matrix_transpose, matrix_product to matrix_utilities.

7740b4d

Deprecation notice for changelog.

27d3930

mhvk force-pushed the coord-no-np-matrix-use-matmul branch from 99e16d1 to 27d3930 Compare July 27, 2016 14:48

eteq merged commit 27d3930 into astropy:master Jul 29, 2016

eteq added a commit that referenced this pull request Jul 29, 2016

Merge pull request #5104 from mhvk/coord-no-np-matrix-use-matmul

dc6a382

Remove use of np.matrix in coordinates, version 2

eteq mentioned this pull request Jul 29, 2016

Fixes to sgr coordinates example #5210

Closed

mhvk deleted the coord-no-np-matrix-use-matmul branch July 29, 2016 20:44

mhvk mentioned this pull request Jul 29, 2016

Remove usage of np.matrix #5088

Closed

This was referenced Aug 10, 2016

Failing RTD build: "No module named matrix_utilities" #5230

Closed

spherical_offsets_to gives error with array: AttributeError: 'Quantity' object has no 'hypot' member #5163

Closed

astrojuanlu mentioned this pull request Feb 4, 2017

Review coordinates transformations for Astropy 1.3 poliastro/poliastro#143

Closed

pllim mentioned this pull request Dec 17, 2019

Improve performance of rotation_matrix for common angle units #9793

Merged

Conversation

mhvk commented Jun 15, 2016

Uh oh!

mhvk commented Jun 15, 2016

Uh oh!

astrofrog Jun 16, 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!

bsipocz Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

astrofrog Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

mhvk Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Jun 16, 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!

mhvk commented Jun 16, 2016

Uh oh!

astrofrog commented Jun 16, 2016

Uh oh!

mhvk commented Jun 16, 2016

Uh oh!

mhvk commented Jun 22, 2016

Uh oh!

StuartLittlefair Jun 24, 2016

Choose a reason for hiding this comment

Uh oh!

mhvk Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

StuartLittlefair commented Jun 24, 2016

Uh oh!

mhvk commented Jun 27, 2016

Uh oh!

mhvk commented Jul 4, 2016

Uh oh!

eteq commented Jul 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jul 5, 2016

Uh oh!

StuartLittlefair commented Jul 5, 2016

Uh oh!

mhvk commented Jul 27, 2016

Uh oh!

adrn commented Jul 27, 2016

Uh oh!

eteq commented Jul 29, 2016

Uh oh!

eteq commented Jul 29, 2016

Uh oh!

eteq commented Jul 29, 2016

Uh oh!

mhvk commented Jul 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

astrofrog Jun 16, 2016 •

edited

Loading

MSeifert04 Jun 16, 2016 •

edited

Loading

eteq commented Jul 5, 2016 •

edited

Loading