Repository navigation
A sequence of rotations in 3D space - #9369
Conversation
astropy-bot
Bot
added
io.misc
zzz 💤 io.misc.asdf
| orig_shape = x.shape or (1,) | ||
| inarr = np.array([x.flatten(), y.flatten(), z.flatten()]) | ||
| result = np.dot(_create_matrix(angles[0], self.axes_order), inarr) | ||
| x, y, z = result[0], result[1], result[2] |
There was a problem hiding this comment.
Is this line necessary? Why not x, y, z = np.dot(...) directly?
| def spherical2cartesian(alpha, delta): | ||
| alpha = np.deg2rad(alpha) | ||
| delta = np.deg2rad(delta) | ||
| x = np.cos(alpha) * np.cos(delta) |
There was a problem hiding this comment.
Please swap alpha<->delta (in line 50)
| return alpha, delta | ||
|
|
||
|
|
||
| class Rotation3D(Model): |
There was a problem hiding this comment.
It may be too late to change the name, but would something like Rotation3DSequence be more descriptive?
There was a problem hiding this comment.
maybe XYZ instead of Sequence. I do not believe it is too late to change the name but isn't Sequence a little bit too Pythonic (=long)?
There was a problem hiding this comment.
Gasp! Is it possible to be too Pythonic? :-)
There was a problem hiding this comment.
A Python code that is too Pythonic? 🤔
Does XYZ mean something to people that do not work in cartesian coordinates, or is this only useful for cartesian?
There was a problem hiding this comment.
I like Rotation3DSequence - although a bit long it it reflects an important feature of the model. I will change to that. I think that 3D in the name already implies XYZ.
There was a problem hiding this comment.
XYZ->3D however, 3D not -> XYZ :)
| ---------- | ||
| angles : array-like | ||
| Angles of rotation in deg in the order of axes_order. | ||
| axes_order : str |
There was a problem hiding this comment.
This seems a bit confusing. I'm not sure what the standard way of distinguishing a constructor parameter from a model parameter is, but one gets the impression that these are both model parameters, when only the first one is.
There was a problem hiding this comment.
It's an unfortunate terminology overload in docstrings and modeling.
| angles : array-like | ||
| Angles of rotation in deg in the order of axes_order. | ||
| axes_order : str | ||
| A sequence of 'x', 'y', 'z' corresponding to axis of rotation. |
There was a problem hiding this comment.
Would be useful to give an example of a value since it may not be obvious what is meant is a string like: "xzy" rather than ['x', 'z', 'y'] even though it says it is a string value.
| # 1-element 1-D array on output for consistency with most other models | ||
| orig_shape = x.shape or (1,) | ||
| inarr = np.array([x.flatten(), y.flatten(), z.flatten()]) | ||
| result = np.dot(_create_matrix(angles[0], self.axes_order), inarr) |
There was a problem hiding this comment.
I'm sure this is my misunderstanding but looking at this, it looks like only one angle is being presented to _create_matrix but a list of axes along with it. What am I missing? And why isn't the model parameter self.angles used here instead? (e.g., why is it an evaluate argument at all?).
There was a problem hiding this comment.
There may be a subtle bug in parameter handling. angles ends up being a 2D array when passed to evaluate. This is limited to rotations and may be partly due to handling of units. I don't have time to debug it now so I took the original value.
| mat = rotations._create_matrix([phi, theta, psi], axes_order) | ||
|
|
||
| assert_allclose(mat.T, matrices[axes_order]) # get_rotation_matrix(axes_order)) | ||
|
|
There was a problem hiding this comment.
Given that the following tests are very JWST specific, perhaps a little context for what v2, v3 means for someone in the future who needs to change tests that isn't familiar with it?
|
|
||
| Examples | ||
| -------- | ||
| >>> model = RotationSequence3D([1.1, 2.1, 3.1, 4.1], axes_order='xyzx') |
There was a problem hiding this comment.
I recommend xyzzy for obscure reasons (just kidding). Google it.
There was a problem hiding this comment.
Or zzyzx. Google it. It may be a good topic for donuts.
perrygreenfield
left a comment
perrygreenfield
left a comment
There was a problem hiding this comment.
LGTM; let me know when you are ready for merging.
|
Ready now, thanks for reviewing! |
4 participants
The schema for this model is implemented in asdf-format/asdf-standard#209