Repository navigation
Allow array with any dimension equal to 3 to initialize CartesianRepresentation - #5439
Conversation
mhvk
changed the title
|
This PR is based on #5434; to see what it adds, just look at the seond commit. The main idea is to have the ability to tell for an input vector array which axis the coordinate triples are stored in. Furthermore, it gives the option to pass in a unit (mostly for regular arrays returned by cc @StuartLittlefair, @eteq. |
eteq
left a comment
eteq
left a comment
There was a problem hiding this comment.
👍 to this, modulo my naming/docs comments below.
| ('z', u.Quantity)]) | ||
|
|
||
| def __init__(self, x, y=None, z=None, copy=True): | ||
| def __init__(self, x, y=None, z=None, unit=None, axis=0, copy=True): |
There was a problem hiding this comment.
Here (and in the get_xyz), I think it might be better to call it something like xyz_axis. With just "axis" it's not really clear at a glance what that means.
There was a problem hiding this comment.
I like it for __init__ certainly. For get_xyz it is a bit less obvious, but for consistency obviously both should be the same. So, done.
| be in this unit if not given. | ||
| axis : int, optional | ||
| The axis along which the coordinates are stored for the case an array | ||
| of vectors is passed in (default: 0). |
There was a problem hiding this comment.
maybe case an array of vectors is passed in -> when a single array is provided rather than distinct x y, and z``? To me, at least, that seems clearer in the context
| y = self.attr_classes['y'](y, copy=copy) | ||
| z = self.attr_classes['z'](z, copy=copy) | ||
| raise ValueError("x, y, and z are required to instantiate {0}" | ||
| .format(self.__class__.__name__)) |
There was a problem hiding this comment.
I think maybe there should be an exception now raised also in the case where axis !=0 but y and z are provided. Right now axis is just silently ignored in that case, right? Catching that will help users understand what the point of axis is.
Also allow extracting such arrays along any axis. And to set a unit.
0de7102 to
da6e898
Compare
da6e898 to
1a7cd3b
Compare
|
@eteq - after a bit of a hassle to understand and solve the numpy 1.7 failures, I now got to implement your comments -- all very good, thanks! With this, I guess ready to merge? |
| earth_heliocentric = (earth_pv[0] - | ||
| sun).get_xyz(xyz_axis=-1).to(u.au).value | ||
|
|
||
| # also prepare earth_pv for passing to erfa. |
There was a problem hiding this comment.
I think this could benefit from the same explanatory comment used in line 124 of icrs_cirs_transforms.py.
| Returns | ||
| ------- | ||
| xyz : `~astropy.units.Quantity` | ||
| With dimension 3 along ``axis``. |
There was a problem hiding this comment.
typo: should be "along xyz_axis"?
|
@mhvk Excellent work! I like the way this closes the loop and means the conversion code is really tidy now. |
|
@StuartLittlefair - OK, good suggestions. I made the changes made. @eteq - since you were happy modulo comments that I addressed, I'll go ahead and merge. Thanks both! |
|
👍 |
3 participants
EDIT: this is now a PR!
For arrays holding
xyzvalues, the dimension that holdsxyzis in numpy often the last, but inCartesianRepresentationwe only easily handle the case where it is the first. In #5434 (comment), it was suggested to consider having a way to handle both cases, perhaps by havingaxis(andunit?) arguments (this would be forCartesianonly).