Skip to content

Fix curved mirror - #2936

Merged
1 commit merged into
mccode-dev:mainfrom
antoinepado:main
Oct 30, 2026
Merged

1 commit merged into
mccode-dev:mainfrom
antoinepado:main

Conversation

@antoinepado

Copy link
Copy Markdown
Collaborator

A few fixes are done.

  • Before assigning a solution ($$l_0$$ or $$l_1$$) to $$d_l$$:
    • If only one solution is found, check it's in the correct half of the cercle (0 to R, not R to 2R) where the mirror is. (Lines 87-89 and 91-93).
    • If two solutions are found, check it's in the right half of the cercle AND if it's within the mirror boundaries (defined by yheight and zdepth). The check for z using the arc's length and the check for y are done in the same way as the one below done at line 113. (Lines 95 and 99).
  • Add condition at line 107 making sure a solution ($$l_0$$ or $$l_1$$) has been found. There is no need to continue the flow of code if no solution is found.
  • Fix condition checking if we are in or out of mirror boundaries at line 113.
  • $$k_{total} = \sqrt{k_x^2+k_y^2+k_z^2}$$ is passed as a parameter in the refleccq function instead of the projection of k in the xz plane $$k_{xz} = \sqrt{k_x^2+k_z^2}$$.

Please let me know if I have made any mistakes or misunderstood something.
Thank you.

@willend

willend commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Thanks @antoinepado, I went through this carefully (generated with Claude Code, checked by @willend). The physics is right, and it also fixes two problems the description doesn't mention. Nothing is needed from your side; we will take care of the follow-ups listed at the end.

What I checked

Change Verdict
Half-circle check |x+knx·l| < |R| for single-root hits Correct for both signs of radius. solve_2nd_order does not sort its roots, so all the sign cases occur.
Two roots: arc-length z check, y check, and trying l1 with the same tests Correct. Previously l1 was taken unconditionally and could lie on the far half of the circle (x > R). y + (ky/k)·l is the right y position, since l is the xz-projected length.
if (dl != 0) guard Correct. It also fixes the both-roots-negative case: the old code called RESTORE_XRAY and then fell through to the bounds check and SCATTER anyway.
Final check zdepth/2 < |radius·alpha| Correct, and this is the change that moves the numbers. The old check zdepth < radius*alpha had no lower bound: every hit upstream of the mirror centre (z < 0) was accepted however far away, and downstream the mirror extended to a full zdepth. The new check matches the %P doc and MCDISPLAY (±zdepth/2).
k_total in refleccq Correct. For COATING, k is the energy used for the table lookup, so rays with ky ≠ 0 were looked up at the wrong energy. The angle is now the true grazing angle. Q = 2|k·n| was already exact, since n has no y component.

Numerical impact

I ran all 39 active %Examples of instruments that use Mirror_curved (1e6 rays, seed 1001), once with the component on main and once with this PR:

  • Unchanged (ratio 1.000, or within 0.4%): Test_Mirrors (all 19), MAXII_711, MAXII_811, MAXIV_FemtoMAX, JJ_SAXS, SOLEIL_CASSIOPEE, SOLEIL_GALAXIES, SOLEIL_MARS, SOLEIL_ROCK
  • Changed (PR/main): SOLEIL_SAMBA 0, SOLEIL_SIRIUS 2e-9, SOLEIL_PUMA 0.013, SOLEIL_PX1 0.10, SOLEIL_CRISTAL 0.25, XFEL_SPB 0.57, SOLEIL_DISCO 0.64, SOLEIL_SWING 1.24, MAXIV_Bloch (E0=0.6) 1.73, SOLEIL_NANOSCOPIUM 2.46 (noisy, ±20%)

These drops are caused by the instruments, not by your fix. I traced SOLEIL_SAMBA ray by ray: every ray picks the same intersection with the PR as on main, and the whole difference comes from the corrected mirror bounds. The instrument relied on the over-long mirror:

  • It computes SAMBA_M1_radius = 28/sin(SAMBA_M1_angle*DEG2RAD) before SAMBA_M1_angle is converted from mrad, so it treats 3 mrad as 3°: R = 535 m instead of about 9333 m. With that much curvature, the rays the DCM accepts were the ones reflected outside the real 1.3 m mirror. Doubling the mirror length restores the DCM intensity.
  • Even with the radius corrected, some rays hit M2 8–18 m downstream of the mirror centre, so M2 is misaligned with the beam. The old code accepted all the upstream misses.

SIRIUS, PUMA, PX1 and CRISTAL show the same pattern (large losses right after the mirror/DCM section).

What we'll do on our side

  • Small style fixes on top of this PR: re-indent the new if (dl != 0) { block to match .clang-format, and remove the whitespace-only line.
  • Fix the alignment and mirror radii of the affected instruments, then update their %Example targets so CI stays green.
  • While at it, also fix what is already wrong on main: the targets of SOLEIL_MARS, SOLEIL_ROCK, SOLEIL_SIRIUS, SOLEIL_SAMBA and SOLEIL_DISCO are far from what main produces, and the SOLEIL_PX2a example passes an unknown parameter sample=.

A small wish for future PRs

When a fix changes simulation results, it would help a lot if the PR already included the consequences: updated %Example targets, and adjustments to instruments that turn out to depend on the old behaviour. Then CI stays green and the PR can be reviewed and merged in one go. A local conda-forge based build makes this fairly quick: see Build your own McStas or McXtrace via conda-forge. After that, mxtest --comp=Mirror_curved runs every instrument that uses the component (see mctest). No problem at all this time, we'll handle it here.

Thanks again for another careful fix!

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