-
Notifications
You must be signed in to change notification settings - Fork 21
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
Update SimCore to include recent features #1235
Update SimCore to include recent features #1235
Conversation
Right, so something was changed by all of this. I'm not sure why but TS and Hcal features seem to have changed but Ecal/other features have remained the same. |
The logs are different as well so something has changed w.r.t. the simulation
|
The first difference occurs at event 150
So here something changed. As far as I can tell, this was the first time that there was a second PN attempt. |
What fun, I can't reproduce either the gold or the PR log results on my machine.... |
On my machine with trunk vs PR branches for a very small ecal pn sample and using https://github.com/tomeichlersmith/root-diff/ LDMX_Events mismatched between files
== Branches with different content ==
EventHeader.timestamp_.fSec
EventHeader.timestamp_.fNanoSec
SimParticles_test.second.processType_ So this looks like what we would expect |
Ok, I've confirmed that the difference comes from the introduction of the KaonPhysics, I will have to figure out what causes it. |
I'm retracting my previous confirmation that the issue was caused by the kaon changes... There was an event seed mismatch between the two branches I was comparing so it doesn't tell us anything. The seed for the previous event however was the same (!?) |
Found the culprit in a comment in a Geant4 header file: // G4DecayTable.hh
// Insert a decay channel at proper position
// (i.e. sorted by using branching ratio ) |
From the G4DecayTableMessenger, the accepted way to change the branching ratio of a process is void G4DecayTableMessenger::SetNewValue(G4UIcommand * command,G4String newValue)
{
if (command == brCmd) {
//Command /particle/property/decay/br
G4double br = brCmd->GetNewDoubleValue(newValue);
if( (br<0.0) || (br>1.0) ) {
G4cout << "Invalid brancing ratio. Command ignored." << G4endl;
} else {
currentChannel->SetBR(br);
}
}
}
} This doesn't change the location of the channel, so according to G4 we should be ok in doing this as well. To be sure, here is the algorithm used to pick decay channels while(true){
G4double sumBR = 0.0;
G4double r= G4UniformRand();
// select decay channel
for (auto channel : channels) {
sumBR += channel->GetBR();
if ( // Not allowed due to parent mass) { continue}
if (r < sumBR) return channel;
}
} This should not depend on the order that the change is made. So we can use the actual Geant4 positions safely. |
Hey whaddya know only the expected to fail plots failed :) |
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.
Everything looks good here, I'm thankful the CI helped us find this very funky bug 💯
I have two house-keeping requests before I'm ready to merge.
- Update SimCore to the v1.8.0 tag (I try to refer to the tag name in the commit message).
- Update the SimParticle DQM to align with the increase in the number of process types. (maybe add the names to the bins as well?)
Were you running a special/nuclear config? I'm just surprised there are so many "unknowns" |
It was a very soft kaon particle gun, so most of the unknown are ionization from hadrons |
But that was what made me catch the pi-inelastic mapping being wrong :) |
Ok, SimCore bumped! |
I am updating ldmx-sw, here are the details.
What are the issues that this addresses?
This resolves #1234
Check List
I successfully compiled ldmx-sw with my developments
I ran my developments and the following shows that they are successful.
See PRs in SimCore
I attached any sub-module related changes to this PR.
Related Sub-Module PRs
If you have developments in a submodule that you need to merge in along with these developments, please link those PRs here, otherwise just delete this section.
Note: You do not need to make a Sub-Module PR if your submodule developments are already on that repo's default branch.
Of these I expect one of the SimObjects DQM plots to fail (because primary particles will be counted as "Primary" rather than "Unknown"). W.r.t. the signal simulation, I leave it to @tomeichlersmith to say if anything is expected to change