Coordinate transformation to Q vector #412
Conversation
src/scippneutron/conversion/tof.py
Outdated
| """ | ||
| e_i = incident_beam / sc.norm(incident_beam) | ||
| e_f = scattered_beam / sc.norm(scattered_beam) | ||
| return 2 * np.pi / wavelength * (e_i - e_f) |
There was a problem hiding this comment.
I am not sure a vector3 dtype is convenient here. Typically, the user will want to bin into the 3 Q components in a subsequent step, so returning Qx, Qy, and Qz may be better?
There was a problem hiding this comment.
After talking to Celine, I decided to switch to returning individual components and drop Q_vec for now as it seems less important. But it is easy to add back in if it turns out that it is useful, e.g. for computing the hkl vector.
| 2 * np.pi / wavelength * sc.vector([0.0, -1.0, 1.0])) | ||
| assert sc.allclose( | ||
| Q_vec['pos', 2], 2 * np.pi / wavelength * | ||
| sc.vector([0.0, -0.4472135954999579, 0.1055728090000841])) |
There was a problem hiding this comment.
As a sanity check, you could compute Q and Q_vec, and check if the norm is consistent?
There was a problem hiding this comment.
Right. Meant to do that and forgot. Thanks!
| Q_vec = sc.vectors(dims=Qx.dims, | ||
| values=np.dstack((Qx.values, Qy.values, Qz.values)), | ||
| unit=Qx.unit) |
There was a problem hiding this comment.
Use https://scipp.github.io/generated/functions/scipp.geometry.position.html?
(I think we should rename this function though, I don't like the name).
There was a problem hiding this comment.
(I think we should rename this function though, I don't like the name).
I guess this is why I missed it
There was a problem hiding this comment.
Ah no, this is not the reason. I missed it because I looked at scipp.spatial. Why do we have both modules? (And geometry is not documented as a separate module.)
There was a problem hiding this comment.
This is just legacy, from before when we had spatial. I think we could have something named spatial.zip and spatial.unzip, maybe? Or spatial.to_vector?
| return 2 * np.pi / wavelength * (e_i - e_f) | ||
| e = e_i - e_f | ||
| k = 2 * np.pi / wavelength | ||
| return k * e.fields.x, k * e.fields.y, k * e.fields.z |
There was a problem hiding this comment.
It is unlikely that this will ever become a performance bottleneck, but it would be interesting to think about scipp::variable::transform that would return multiple Variable.
Part of #411