Union refactoring of scattering while loop

[Lomholy]

Free-form text area

Please describe what your PR is adding in terms of features or bugfixes:
This PR aims to improve the readability of the Union_master component. The PR is limited in scope to only the scattering while loop inside Union_master.comp.

To perform this refactoring I propose two things:

1. We separate all non-primary logic paths, out into separate functions (Currently placed in Union_master for my own simplicity, but will be moved to union-lib.c).

2. When determining a non-primary logic paths, we use wrapper functions, that clearly indicate the intent of the branching in logic path.
   An example of this is using:
   if (volume_is_only_absorber(Volumes[current_volume])) {
   Instead of
   if (Volumes[current_volume]->p_physics->number_of_processes == 0) { // If there are no processes, the volume could be vacuum or an absorber
if (Volumes[current_volume]->p_physics->is_vacuum == 0) {

3. (Maybe) We attempt to adhere to a standardized naming scheme for the functions implemented, such that all inhomogenous paths are called "inhomogenous_insert_what_function_does();".

---

Declaration of use of AI-tools

• Please add a checkmark here if you used AI-tools during the work for this contribution
• Further, please describe how / where and for what the tools were used:

---

Development OS / boundary conditions

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

---

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 work touches / adds to the runtime lib code (.c,.h etc in multiple locations

  • I am have added reasoning and documentation for the change below
  • I am attaching test output in the comments

---

[mads-bertelsen]

This is an ongoing review, but want to get some information across!

The trace loop is certainly easier to read with this. Naming the functions in this complex system is difficult, will think more about how exactly that would be the most intuitive. It's mainly confusing when reading the functions before reading the trace section, which of course is not often done and can be alleviated with a header.

The function calls use pointers for everything, that reduce the overhead on function calls as nothing needs to be copied. It does make the syntax in the functions a bit harder to understand due to all the dereference operations. When a parameter is not updated in a function, one can make a local variable for the dereferenced value and use that in the equations, it should be optimized out by the compiler while being slightly more readable.

Even with the fast function calls, I do want to check the performance impact, although I don't expect it to be significant.

[Lomholy]

The trace loop is certainly easier to read with this. Naming the functions in this complex system is difficult, will think more about how exactly that would be the most intuitive. It's mainly confusing when reading the functions before reading the trace section, which of course is not often done and can be alleviated with a header.

This is true. We might also think about grouping them together, such that all functions "belonging" to a type of logic, sit close to each other in union-lib. Looking forward to hearing your ideas regarding naming!

The function calls use pointers for everything, that reduce the overhead on function calls as nothing needs to be copied. It does make the syntax in the functions a bit harder to understand due to all the dereference operations. When a parameter is not updated in a function, one can make a local variable for the dereferenced value and use that in the equations, it should be optimized out by the compiler while being slightly more readable.

I was considering if we should make som helper structs, such that the function calls inside the union_master wouldn't have to be so lengthy. By doing this I also believe these dereferencing issues could be adressed to some degree.

Even with the fast function calls, I do want to check the performance impact, although I don't expect it to be significant.

Sounds like a good plan. You never know what operations you are by accident performing!

[willend]

@Lomholy the macOS test will very likely run through OK if you update the branch against current main.

(Seems to be a specific issue with mpi that I will resolve separately later.)

[willend]

@Lomholy and @mads-bertelsen I've just updated against main. Is the status still [NOT READY FOR MERGE]?

[Lomholy]

@Lomholy and @mads-bertelsen I've just updated against main. Is the status still [NOT READY FOR MERGE]?

@willend I am seeing @mads-bertelsen next week, where I think we will have a chat on it! Will update the PR after this.

[Lomholy]

I did find a few issues and a few things I simply did not understand.

Some of them are the lack of access to the _particle, that is necessary for random numbers to function correctly, but > the tricky thing is that it seems to work just fine without them, but the user looses the control of the seed.

Then there is one equation that seems off, but might be a trick I didn't manage to understand.

I think I have responded to all your points, and you were right on all of them. There is one bit that is wrong with the inhomogeneous sampling of processes that need_focus, see my comment to your comment for that one. But since this issue is not relevant to the current PR, I propose that we add an issue regarding it and then let this PR branch finish up if all tests are passed.

[Lomholy]

@willend I think I have now fixed the errors causing the tests to fail, but I cannot test it, when I (with a fresh build) do mctest --testdir tmp --comp Inhomogenous_incoherent_process I get, the following (slightly redacted)

loading system configuration
Output of test will be placed in: tmp
MCCODE root dir ..../mcstas-dev/share/mcstas/resources contains mccode.sim data (extra input files?) This may lead to errors/warnings...
ncount is: 1e6
Testing: 3.99.99

Finding instruments in: .../mcstas/resources/examples
(Instrument list includes those using component(s) Inhomogenous_incoherent_process )
Copying instruments to: tmp/mcstas-3.99.99_Inhomogenous_incoherent_process_1e6_Darwin_20261008_1519_21

WARNING: Skipped Test_inhomogenous_process test - did tmp/mcstas-3.99.99_Inhomogenous_incoherent_process_1e6_Darwin_20261008_1519_21/Test_inhomogenous_process exist already??


Compiling instruments [seconds]...
Running tests / getting status...
Test_inhomogenous_process  :   Display FAILED (0s)
Test_inhomogenous_process  : RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=thin -n1e6 -d1 > run_stdout_1.txt 2>&1
Test_inhomogenous_process_2: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=union_inc -n1e6 -d2 > run_stdout_2.txt 2>&1
Test_inhomogenous_process_3: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=union_inho -n1e6 -d3 > run_stdout_3.txt 2>&1
Test_inhomogenous_process_4: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=inc_linear -n1e6 -d4 > run_stdout_4.txt 2>&1
Test_inhomogenous_process_5: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=inho_linear -n1e6 -d5 > run_stdout_5.txt 2>&1
Test_inhomogenous_process_6: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=inc_and_inho -n1e6 -d6 > run_stdout_6.txt 2>&1
Test_inhomogenous_process_7: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=phonon thick=0.001 -n1e6 -d7 > run_stdout_7.txt 2>&1
Test_inhomogenous_process_8: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=inc_trans -n1e6 -d8 > run_stdout_8.txt 2>&1
Test_inhomogenous_process_9: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=inho_trans -n1e6 -d9 > run_stdout_9.txt 2>&1
Test_inhomogenous_process_10: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=gauss_heavy -n1e6 -d10 > run_stdout_10.txt 2>&1
======================================
Overall test result:
FAILED! One or more tests errored (0 compile errs / 10 runtime errs / 0 values off)

Have you seen this before on an apple M4 max laptop?

[willend]

This here is an indication of overlap with existing test output - maybe from your $MCSTAS folder or Untracked files in your repo checkout?
WARNING: Skipped Test_inhomogenous_process test - did tmp/mcstas-3.99.99_Inhomogenous_incoherent_process_1e6_Darwin_20261008_1519_21/Test_inhomogenous_process exist already??

This on the other hand is most probably a segfault / non-zero exit value. Check the stdout files in the output folder and lint the code
Test_inhomogenous_process_10: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=gauss_heavy -n1e6 -d10 > run_stdout_10.txt 2>&1

[Lomholy]

This here is an indication of overlap with existing test output - maybe from your $MCSTAS folder or Untracked files in your repo checkout? WARNING: Skipped Test_inhomogenous_process test - did tmp/mcstas-3.99.99_Inhomogenous_incoherent_process_1e6_Darwin_20261008_1519_21/Test_inhomogenous_process exist already??

Aha, this confused me as I always try to keep my tests in a tmp folder which I clear everytime.

This on the other hand is most probably a segfault / non-zero exit value. Check the stdout files in the output folder and lint the code Test_inhomogenous_process_10: RUNTIME ERROR, mcrun --no-mpi -s 1000 Test_inhomogenous_process sample=gauss_heavy -n1e6 -d10 > run_stdout_10.txt 2>&1

I wiped my old install, and installed conda anew, and then the tests came through a little better. It says that there is a display which fails, but when I run it normally then it doesn't so I am a little bit confused there.

[willend]

@Lomholy there are separate logs for compilation / display / and each %Example: in the relevant 'instrument' folder within your test output, see e.g. https://new-nightly.mcstas.org/todays-datafiles/mcstas-3.99.99_openacc_mpi_x_8_1e7_Linux_20261009_0001_20/BNL_H8/

[Lomholy]

@willend I tried to examine the log for the display but it seems that none is written?

[willend]

Hmm, weird. Try going in the folder to do these

• mcdisplay-webgl-classic Test_inhomogenous_process.instr -y
• ./Test_inhomogenous_process.out --trace -n100 -y ? (Maybe redirect to file with > my_file or use an even lower -n)

It could be some var in a component DISPLAY section is un-initialised somewhere... Smells like segfault or other low-level error.

[Lomholy]

It works when I do it by hand with mcdisplay-webgl-classic Test_inhomogenous_process.instr -y

[Lomholy]

* `./Test_inhomogenous_process.out --trace -n100` -y ? (Maybe redirect to file with `> my_file` or use an even lower `-n`)

This also works fine by hand btw

[willend]

Yes but you have to figure out if there is a non-zero exit value of mcdisplay itself / the simulation.
echo $?

[willend]

The noted "error" comes from a non-zero exit value.

[Lomholy]

It seems that the issue was an old .out file I had lying in my Test folder inside the local repository not inside the mctest folder inside tmp. For some reason it found that one and used it, but didn't even attempt to display. Trying to run again now, and seeing if it all works

[Lomholy]

There we go:

[Lomholy]

@mads-bertelsen I think a final cursory glance from you would be good before merging, but otherwise it looks good.

It doesn't seem like the failing tests have anything to do with this PR, right?

[willend]

@Lomholy yes this would explain the issue - (And is the reason the Warning about simulation data / blah comes up... I am considering to upgrade that to an actual error / crash instead. You have to know what you are testing.)

The mcstas-antlr issue should not be related but I the 'comparison' issue should be investigated - however chances are it will all go away with a rebase / update via merge request. Do you want me to do that?

[willend]

@mads-bertelsen I think a final cursory glance from you would be good before merging, but otherwise it looks good.

It doesn't seem like the failing tests have anything to do with this PR, right?

Indeed @Lomholy I have marked all of the conversations above as 'resolved' but @mads-bertelsen still needs to flick the toggle to "approved". And we of course need to still await 'green light' wherever we can get it.

[mads-bertelsen]

Great work, will look through it as soon as possible!