-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Grabmayrs gamma-cascade-reader and a custom neutron capture process #123
Conversation
Did you test this with MT mode on? I do not suppose that all the file-reading stuff is thread-safe, or is it? (making the reader instance thread-local should be enough. It has to be constructed on each thread anyway, otherwise the registered commands will not really work in some edge cases.) |
I did test it, thats why in the UserAction the instance is called in the This worked for my tests, but i have to admit i am not too sure if that solution works for all edge cases, as i am not entirely sure about the interaction of singletons in MT mode. |
I do not have time to test it now, but I would guess that the current code will fail if you move I would be more concerned with only using one global instance without explicit locking, when it comes to file reading. Afaik, the C++ standard does not guarantee thread-safety around stream objects (see section 30.2.3 in the C++ 17 draft)
I think, The initialization will be mostly work fine, even though technically it is UB, I suppose. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work (not compile-tested though).
This fail seems to be totally unrelated to the changes commited? (During runtime all of the added code is inactive if the macro is not set) |
Yes, it's just the typical test being flaky, as G4 11.0 has apparently some navigation bugs :-) |
Now it really looks better. I still have some minor (mostly cosmetci) remarks, then it should be good from my side :-) |
src/RMGGrabmayrGCReader.cc
Outdated
|
||
RMGGrabmayrGCReader::GCMessenger::GCMessenger(RMGGrabmayrGCReader* reader) : fReader(reader) { | ||
fGammaFileCmd = new G4UIcommand("/RMG/GrabmayrGammaCascades/SetGammaCascadeFile", this); | ||
fGammaFileCmd->SetGuidance("Set the Z, A and /path/to/file for the gamma cascade employed upon " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really like the message here. It seems to technical for a general description of what this command does. Maybe something akin "Set a gamma cascade file to be used upon neutron capture on a specifc isotope"?
(The parameters are already described by their own docstring, thats fine.)
I tried to group a manual edit fix together with adding the suggestions in one commit purely on the web-interface, which i guess didnt work^^ (I have to redo the doc-dump anyways so) |
Should be fine now. Anyways, thanks for the feedback and putting up with my code :) |
The first commit is intended to add a refurbished version of the gamma-cascade-reader originally written by @MoritzNeuberger for WLGD. It employs a custom
RMGNeutronCaptureProcess
inheriting fromG4HadronicProcess
and replacing the nativeG4NeutronCaptureProcess
. This process is supposed to employ the gamma cascades from Peter Grabmayr (https://link.springer.com/article/10.1140/epjc/s10052-023-11602-y) upon neutron capture on specified Isotopes, otherwise keeping the standard process. It manually employs the cross section datasets that are usually employed withShielding
and takes over all of the interactions from the nativeG4NeutronCaptureProcess
. Works with 3 of the 4 physics list, asQGSP_BERT_HP
does employ a different process for "nCapture" (although this could be fixed with some investigation, for now this will result in just the native process being used and an error message thats not crashing the application, if theUseGrabmayrsGammaCascades
macro is set).There are certainly other options for this feature, but overall to me this seems like a "smoother" solution, than doing it in the tracking action like in WLGD. However, if there are other ideas that suit the concept of ReMaGe more, i am open ears. This is just a proposal.
The second commit provides an example to test this new feature, while employing a user-defined optional output scheme to write out all of the isotopes created through
nCapture
orRMGnCapture
. It provides two macros to compare the native result to this new custom version.The example does not need to be pulled, but it allows for quick tests (and shows how a user-defined optional output scheme would work)