Repository navigation
Conversation
On SIGINT/SIGTERM, stat:sum:initial_ray_count was the requested ncount, so intensities read from the file were too low (mccode-dev#3055). It is now the number of rays simulated up to the last recorded particle (from _uid), or left at -1 if nothing was recorded or with OpenACC. A warning is printed in both cases. MCPL_output_noacc: default weight_mode=1, as MCPL_output. Add test_mcpl_interrupt.py and run it in the basictests. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Interrupt only once 1 MB of MCPL output is on disk, and print the exit status and the instrument output when the check fails. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
There the instrument is killed by SIGINT without the runtime signal handler running, so the final save under test never happens. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Looks sensible I guess, although the changes are more intrinsic to how mccode operates than how mcpl works, so I could easily miss something. One thing stood out to me, namely: I also looked at MCPL_output.comp, and my concern remains: Having zero particles hitting a detector after 1000 rays coming out of the source is not the same as "I crashed and didn't have a chance to update the file", is it? In case the user ran -n 1000 and had all rays absorbed before getting to MCPL_output.comp we would expect an initial_ray_count of 0, so why should it be -1 after a ctrl-c which lead to the same conditions? |
|
Good point, will apply a relevant fix in this direction. |
|
Correction to what I wrote: In case the user ran -n 1000 and had all rays absorbed before getting to MCPL_output.comp we would expect an initial_ray_count of 1000 (not 0 like I wrote). |
|
Thanks for the correction, and I agree in principle: the ideal number is the count of rays actually simulated before the interrupt. The catch is that MCPL_output can’t know that number. It only sees the rays that reach it, and McCode keeps no running ray count that is valid when a run is interrupted. So I’d rather keep this fix inside the component. What it writes is the number of rays up to the last particle recorded, so the file is consistent as “a run that stopped at that particle”. The rays simulated after it are not counted, which is a small error, well below the statistical one. Compared to today, where an interrupted file silently claims the full requested ray count, I think this is a good trade. That leaves the case you raised, an interrupted run where no particle was recorded at all. We can’t write the true count there, so it is a choice between two values, and I’d like your preference from the MCPL side: 0: the same rule as above taken to its limit, giving a file with 0 rays and 0 particles. It is valid and adds nothing when merged with other files, but the rays that were simulated without a hit are lost. Which of the two would you rather see in such a file? For completeness: the exact count could be had by adding a ray counter to the main loop that the code generator writes. We would like to avoid that. It changes the generated code of every instrument, and it has to be made correct for MPI, OpenACC and the FUNNEL mode, all to improve a corner case of an interrupted run. If the approximate count turns out not to be good enough in practice, we can revisit it as a separate change. |
|
My own preference would probably be something like:
|
|
TLDR (all). But what is I would avoid any sort of "fall back to 0' since that defeats the entire purpose of how stat:sum's are supposed to be implemented. |
|
We originally had a function |
The count starts at 0, is set to -1 just before the first particle is written, and gets its final value in FINALLY: ncount for a completed run, the simulated count for an interrupted one (-1 with OpenACC). An interrupted run that stored nothing now carries 0 instead of -1. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
Pushed 8fb8bd2.
So 0 only ever appears in a file without particles. PR description updated to match. |
|
So for the record, I think it is important for consistency with all the other work going into this that any file that does not have access to the correct value puts a -1 rather than 0 into the stat:sum. A very short simulation in which the user hit ctrl-c before any particles are entered, are also unlikely to have a big file that is crucial to keep around. |
|
You seem to be contradicting yourself - and you have not understood what I wrote above:
Absorbed particles don't arrive. If all rays are absorbed before getting to Hence, without a code-generator change the only setup that will make sense is what I proposed / committed above: Pushed 8fb8bd2. stat:sum:initial_ray_count now evolves like this: 0 until the first particle is stored. With no rays stored there is no weight to renormalise, so 0 rays / 0 particles is a valid statement about the file. The only alternative is to have it always be at -1 - which will make an empty file / no particles stored will look like a crash: |
|
Or completely lazy / unclever: -1 whenever the full stat is not reached. Which again has the side-effect that a file without particles will look like the simulation crashed, no matter if it actually crashed. Granted, I like my solution above better, but I also dislike repeated misunderstandings / discussing the same point ad infinitum in a comment thread. Timeout, talk IRL. |
Free-form text area
Fixes #3055.
When a run was interrupted (SIGINT/SIGTERM),
MCPL_outputstill wrote the requested ncount asstat:sum:initial_ray_count, so the file looked complete and intensities derived from it were too low.Changes, in McStas
MCPL_output,MCPL_output_noaccand McXtraceMCPL_output:initial_ray_countis the number of rays simulated up to the last recorded particle, taken from_particle->_uid. The stored weights arep * ncount_requested, so no rescaling is needed.-1(unavailable) just before the first particle is written, and gets its final value inFINALLY. With OpenACC, where_uidorder is not execution order, an interrupted run with stored particles keeps-1.Warning (vout): run interrupted, initial_ray_count set to 4353479 of 2000000000 requested rays.initial_ray_countFINALLY(also: hard kill)MCPL_output_noacconly: defaultweight_modeis now 1, as inMCPL_outputand as its header already documented (it used to fall back to 2 with a warning). This changes the default output of that component.New regression test
test_mcpl_interrupt.pynext toTest_MCPL_output.instr(identical copies for McStas and McXtrace), run from the basictest workflows (main job; passes there on Linux and macOS). It interrupts a run and checks that the ray count matches the number of particles. It skips on non-POSIX systems.Notes / limitations:
mpirunkills the ranks while they compress their files, so an interrupted MPI run leaves per-rank.mcplfiles and no merged file. Not addressed here.Declaration of use of AI-tools
Development OS / boundary conditions
Developed and tested on macOS arm64 (clang, conda
mccode-devenvironment), CPU only. Linux and macOS are covered by CI through the new test. Not tested on Windows (test skips), FreeBSD or GPU/OpenACC. The test needsmcpltool.Tested locally:
test_mcpl_interrupt.pyfails with the unpatched component and passes with the patched one, McStas and McXtrace.-np 2.MCPL_output_noacc: completed and interrupted runs write the expected count.PR Checklist for contributing to McStas/McXtrace
My contribution includes patches to an existing component file
mcdocutility and rendered a reasonable documentation page for the component (please attach as screenshot in comments!)mctestutility to test one or more instruments making use of the component (please attachmcviewtestreport as screenshot in comments)mccode-clangformattool to apply the standard McCode component indentation schememcrun --c-lint"linter" and followed advice to remove most / all warnings that are raisedMy PR is meant to fix a specific, existing issue
My contribution contains something else
🤖 Generated with Claude Code