Repository navigation
Use representation arithmetic - #5434
Conversation
| representation = representation.represent_as(UnitSphericalRepresentation) | ||
| else: | ||
| representation = CartesianRepresentation(x=x*c.unit, y=y*c.unit, z=z*c.unit) | ||
| r = r.represent_as(UnitSphericalRepresentation) |
There was a problem hiding this comment.
I realize you're following r from some of the other cases here, but what about uniformly changing to representation instead of r? I remember when writing some of this that I avoided r because it seemed like r as in the spherical coordinate r rather than "representation".
There was a problem hiding this comment.
I made it rep, representation being a bit too long...
731f70d to
534314b
Compare
|
OK, rebased this, and replaced |
StuartLittlefair
left a comment
StuartLittlefair
left a comment
There was a problem hiding this comment.
This is lovely @mhvk!
| newrep = CartesianRepresentation(newxyz).represent_as(SphericalRepresentation) | ||
| astrom_eb = CartesianRepresentation( | ||
| u.Quantity(np.rollaxis(astrom['eb'], -1, 0), u.au, copy=False), | ||
| copy=False) |
There was a problem hiding this comment.
This construct is repeated often enough to make me think it's worth hiving off into a utility function.
There was a problem hiding this comment.
Yes, I wondered also whether to make the CartesianRepresentation initializer a little more flexible, so one could give an array and just tell it which axis to take as the xyz one (rather than only allowing axis 0). One might then also want to be able to pass in a unit...
There was a problem hiding this comment.
I raised #5439 to remind myself, as I'm not sure I'll have time to do it here (or whether indeed it isn't better done separately).
534314b to
fc5f5b8
Compare
|
@eteq - this now passes. Let me know if you have any remaining comments on this (or if there still are problems with multi-D representations). |
|
@mhvk - I'm still getting errors when I try to run eteq/astropy@ec9cba8 locally (in a few different configurations) that look like broadcasting problems. I think the best way to proceed is for me to make a new PR with just that so we can spec out on travis what the problem is, which I did in #5441. But the fact that these tests are currently passing means that it's not seriously affect the existing machinery. So I think it's safe to merge this now anyway... so I will do so! |
4 participants
As it seems the representation arithmetic is nearing merging, I thought it would be useful to see how much simpler the coordinate transformation code becomes if one uses it. I'm quite pleased: now almost all fiddling with dimensions is to get stuff to and from
erfa.Note: for now built on top of #5301; just look at the last commit only to see the effect on the transformations.
p.s. The only part I did not touch is
matching.py.