Skip to content

feat: align the beam-aligned coordinate system with the incident beam - #725

Open
SimonHeybrock wants to merge 2 commits into
mainfrom
beam-aligned-frame
Open

feat: align the beam-aligned coordinate system with the incident beam#725
SimonHeybrock wants to merge 2 commits into
mainfrom
beam-aligned-frame

Conversation

@SimonHeybrock

@SimonHeybrock SimonHeybrock commented Aug 26, 2026

Copy link
Copy Markdown
Member

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 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 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_theta and phi relative to a tilted incident beam, let it fall onto a detector, and require that both angles come back.

scattering_angle_in_yz_plane continues 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 wrong phi/Qx/Qy for any vertical component of the beam center, so ESSsans instead shears every pixel's scattered beam: _scattered_beam_with_beam_center in 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_gravity raise for non-orthogonal beams the way scattering_angle_in_yz_plane already does, which is self-consistent but leaves SANS with the shear. I think generalizing is better: phi is 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?

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
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
SimonHeybrock marked this pull request as ready for review August 28, 2026 05:01
@SimonHeybrock
SimonHeybrock requested a review from jl-wynen August 28, 2026 05:01
@jl-wynen

Copy link
Copy Markdown
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?

Base automatically changed from fix-gravity-drop-direction to main August 31, 2026 04:19
@SimonHeybrock

Copy link
Copy Markdown
Member Author

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?

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 beam_aligned_unit_vectors and BeamAlignedUnitVectors, not CoordSystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants