feat: align the beam-aligned coordinate system with the incident beam - #725
Open
SimonHeybrock wants to merge 2 commits into
Open
feat: align the beam-aligned coordinate system with the incident beam#725SimonHeybrock wants to merge 2 commits into
SimonHeybrock wants to merge 2 commits into
Conversation
The z-axis was defined by projecting the incident beam onto the plane perpendicular to gravity, and the y-axis was pinned antiparallel to gravity. The resulting system is therefore only aligned with the incident beam when the beam is perpendicular to gravity. Otherwise `phi` is the azimuth about the horizontal projection of the beam, which is not a scattering azimuth, and the transverse components of the scattered beam retain part of the beam's inclination. The z-axis is now the incident beam itself and the y-axis is derived from it. The x-axis is unchanged: it is horizontal and perpendicular to the beam either way. Both definitions agree exactly when the incident beam is perpendicular to gravity, so no beamline in use today is affected. This makes it possible to describe a beam whose direction deviates from the nominal source-sample axis by passing the actual direction as `incident_beam`, which is how SANS wants to apply a beam center. `scattering_angle_in_yz_plane` keeps rejecting non-orthogonal incident beams. Its angle is measured from the horizontal plane rather than from the incident beam, so the generalization does not carry over to it.
SimonHeybrock
marked this pull request as draft
August 26, 2026 12:26
SimonHeybrock
added a commit
to scipp/ess
that referenced
this pull request
Aug 26, 2026
The beam center is a point on the actual beam, measured from the sample, so it defines the beam direction. Passing that direction as `incident_beam` says so directly. It replaces the shear, which mapped the actual beam onto the nominal axis by displacing every pixel's scattered beam in proportion to its distance from the sample. The shear is exact along the beam and first order elsewhere, since it is a shear rather than a rotation. For the Loki-at-Larmor detector that costs a relative error in two_theta with a median of 3.8e-4 and a maximum of 1.2e-3, well above the 1e-5 the docstring claimed. Rotating the incident beam is exact everywhere, and it puts phi, the cylindrical coordinates and two_theta on the same footing instead of relying on the shear having mapped the transverse plane closely enough. The reference results move accordingly and are regenerated. Requires the beam-aligned coordinate system to follow the incident beam rather than its projection onto the horizontal plane; see scipp/scippneutron#725. Until that is released, phi and Qx/Qy would drop the vertical part of the beam center.
SimonHeybrock
marked this pull request as ready for review
August 28, 2026 05:01
Member
|
I'm not sure it is a good idea to tilt the coord system. This makes it less intuitive to reason about where components are placed. Can we adjust how we compute phi instead? |
Member
Author
Are we really tilting the coord system? What is computed here is (as far as I can tell) only used for computing scattering downstream, not relating to other component positions. And the naming is already |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #724 — its tests need the corrected drop direction.
The beam-aligned coordinate system defines the z-axis by projecting the incident beam onto the plane perpendicular to gravity, and pins the y-axis antiparallel to gravity. It is therefore aligned with the incident beam only when the beam is perpendicular to gravity. Otherwise
phiis the azimuth about the horizontal projection of the beam, which is not a scattering azimuth, and the transverse components of the scattered beam keep part of the beam's inclination.The z-axis now is the incident beam, and the y-axis is derived from it. The x-axis is unchanged — it is horizontal and perpendicular to the beam under either definition. The two definitions agree exactly for an incident beam perpendicular to gravity, so nothing in use today changes; the existing tests pass untouched.
The new round-trip test states the intent: send a neutron away from the sample at a known
two_thetaandphirelative to a tilted incident beam, let it fall onto a detector, and require that both angles come back.scattering_angle_in_yz_planecontinues to reject non-orthogonal incident beams. Its γ is measured from the horizontal plane rather than from the incident beam, so this generalization does not carry over — the two techniques genuinely want different references, and they coincide only in the orthogonal case.Why
SANS applies a beam center: the beam passes through the sample but its direction, set by the collimation, deviates from the nominal source–sample axis. The natural expression of that is to pass the actual direction as
incident_beam. Today that silently produces a wrongphi/Qx/Qyfor any vertical component of the beam center, so ESSsans instead shears every pixel's scattered beam:_scattered_beam_with_beam_centerin scipp/ess#729. That is exact along the beam and approximate elsewhere, and it is 30 lines of subtle code that this change removes.@jl-wynen — flagging you directly, since this changes a documented convention you defined. The alternative is to make
scattering_angles_with_gravityraise for non-orthogonal beams the wayscattering_angle_in_yz_planealready does, which is self-consistent but leaves SANS with the shear. I think generalizing is better:phiis only ever used as an azimuth about the beam, and refusing leaves a public frame that provably cannot describe the case it is asked about. What do you think?