Skip to content

Conversation

@willend
Copy link
Contributor

@willend willend commented Dec 15, 2025

Free-form text area

Please describe what your PR is adding in terms of features or bugfixes:

This PR makes PowderN reflection sorting perform alike on all platforms:

  • Use direct comparison instead of subtraction (higher precision)
  • As qsort does not guarantee returned order in case of equal element
    • sort on F2
    • sort on j

Has been tested to perform identically within Test_PowderN but could potentially solve differences seen in more complicated powder instruments such as ILL D1B and D2B.

Fixed for McStas + McXtrace PowderN + McStas Union Powder_process

Background data:
Prior to the fix, two powder lines in Fe.laz / Test_PowderN would for instance be swapped unix vs windows:

image

Development OS / boundary conditions

Please describe what OS you developed and tested your additions on, and if any special dependencies are required:


PR Checklist for contributing to McStas/McXtrace

For a coherent and useful contribution to McStas/McXtrace, please fill in relevant parts of the checklist:

  • My contribution includes patches to an existing component file

    • I have used the mcdoc utility and rendered a reasonable documentation page for the component (please attach as screenshot in comments!)
    • I have ensured that basic use of the component is OK (e.g. an instrument using it compiles?)
    • I have used the mctest utility to test one or more instruments making use of the component (please attach mcviewtest report as screenshot in comments)
    • I have used the mcrun --c-lint "linter" and followed advice to remove most / all warnings that are raised
  • My contribution includes patches to an existing instrument file

    • I have used the mcdoc utility and rendered a reasonable documentation page for the instrument (please attach as screenshot in comments!)
    • I have used the mctest utility to test the instrument (please attach mcviewtest report as screenshot in comments)
    • I have used the mcrun --c-lint "linter" and followed advice to remove most / all warnings that are raised

* Use direct comparison instead of subtraction (higher precision)
* As qsort does not guarantee returned order in case of equal element
  - sort on F2
  - sort on j
@willend
Copy link
Contributor Author

willend commented Dec 15, 2025

(We ignore McXtrace / windows since ref-lib uses complex numbers - relevant edits are not in place yet for that case.)

@willend
Copy link
Contributor Author

willend commented Dec 15, 2025

Initial test-run revealed that the target value for D1B needed shifting - also included in the PR
Screenshot 2025-12-15 at 16 16 58

@willend
Copy link
Contributor Author

willend commented Dec 15, 2025

After updated test-value, the output for ILL D1B looks good:
Screenshot 2025-12-15 at 16 44 38

Test output for Powder_process looks good:
Screenshot 2025-12-15 at 16 41 17

Test output for McStas PowderN has one test that is "off" - Test_Powders no. 8 - will require a closer look. Otherwise good:
Screenshot 2025-12-15 at 16 49 14
Screenshot 2025-12-15 at 16 42 55
Screenshot 2025-12-15 at 16 43 04

For McXtrace, PowderN can not be fully tested on Windows (mingw-gcc setup lacks dependencies and conda-setup requires more work for complex numbers.) One test is off for FluoPowder.
Screenshot 2025-12-15 at 16 54 31

@willend
Copy link
Contributor Author

willend commented Dec 16, 2025

Interestingly, it seems that "some undefined behaviour" remains - but quite rare... Two runs of the test-suite from my branch last night - one of them is off, the other is not.
Screenshot 2025-12-16 at 09 22 36
Screenshot 2025-12-16 at 09 20 21
Screenshot 2025-12-16 at 09 22 19
Screenshot 2025-12-16 at 09 20 57

(Seems consistent with current and earlier nightlies on main, see e.g.
https://new-nightly.mcstas.org/2025-12-16_output.html
https://new-nightly.mcstas.org/2025-11-13_output.html
https://new-nightly.mcstas.org/2025-10-06_output.html - test 22 on GPU might worth a specific look)

Copy link
Contributor

@mads-bertelsen mads-bertelsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that the table sorting is now platform independent. Makes sense that without this, same seed runs could differ even though they should converge to the same value in long enough runs.

@willend willend merged commit 0322ddc into mccode-dev:main Dec 16, 2025
17 of 18 checks passed
willend added a commit to willend/McCode that referenced this pull request Dec 16, 2025
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