Closed EinarElen closed 1 year ago
Yeah, I wanted to get it up so people can have a look at it. I'll go ahead and set up the documentation (and something for the website) tomorrow :)
I think enforcing basing it on a simulator makes sense, the user can always change things after creating it (a use case would be resimulating with a slightly different hcal geometry).
Yes, I've checked that it only does the subset you ask for (all others are aborted). One caveat is that if you have multiple input files with the same event numbers, it will resimulate both. You could probably fix that but I don't know if it is worth the effort, since resimulating individual events isn't a super compute intensive effort :)
Since I was touching some major parts of SimCore anyways and runnings formatting, I figured I might as well take the time to make a small dent in https://github.com/LDMX-Software/ldmx-sw/issues/1176 and https://github.com/LDMX-Software/ldmx-sw/issues/1168. What I've done is to build SimCore with both Clang, GCC such that no warnings originate from within SimCore. I then applied clang-format to all translation units that I had touched in this PR (including fixing warnings).
Here's a brief summary of the changes
const
from by-value return types (these do not actually do anything)I could include these in this PR or I could make a separate one (we should run the CI anyway). I tried to make sure that all commits only include one type of change, so it should be possible to review them individually (the formatting of course changes a lot of stuff). I could run clang-format on the whole thing if we want.
All of these are straight-forward changes. However, there are some things that I would love to hear @tomeichlersmith's thoughts on.
G4DarkBremsstrahlung
object in APrimePhysics. At least as far as I can tell, its destructor is never called (even at the end of the program). This probably doesn't matter for us but I thought it would be worth mentioning EndOfEvent
. One is the one we are inheriting from Geant4 but the other one is just for us (called at the end of a Simulator event, rather than at the end of a G4 one). Both Clang and GCC complain about this (-Woverloaded-virtual
because they think it makes it easy to mix them up. I would tend to agree, I actually thought that we were overriding the G4 one. I think the solution is either to rename the LDMX-specific one or to disable the warning. I would prefer the former, what do you think? Finally, to make a very basic test of one of the intended use-cases, I tried running a resimulation with 24 mm absorbers for the back hcal which produced events with the same PN interaction.
I also added some draft documentation over at https://github.com/LDMX-Software/ldmx-software.github.io/blob/resimulator/docs/Generating-Simulation-Samples.md
We are currently leaking the G4DarkBremsstrahlung object in APrimePhysics.
Sigh I guess I'm not surprised. G4DarkBremsstrahlung registers with Geant4's internal process table for electrons during its construction and I naively assumed that the table would cleanup the processes it holds. I think wrapping the process in a unique ptr held by the APrimePhysics class would be sufficient, but we should check a few different runs to make sure it doesn't crash at the end of running. Since we never use the proc
variable, we could also just have a bare new G4DarkBremsstrahlung
(this is unfortunately common in Geant4 physics constructors).
In the SensitiveDetector library, we are defining two virtual functions called EndOfEvent.
I knew this design decision would come back to bite me in the butt. I'm happy to rename the LDMX-specific one - maybe we just call it EndOfFireEvent
? Idk, part of the reason I did it like this is to have users avoid using the G4 one and use ours instead.
I'll give storing it in the constructor a try!
Maybe OnEndOfEvent
or OnFinishedEvent
or something along those lines?
Moving it into a unique_ptr in the constructor worked like a charm :)
There was one more report about a memory leak in the AuxInfoReader
. I've checked and it is a false positive. I tried really hard to convince the linter that it is fine, Geant4 handles the memory but in the end had to mark that part of the code as NOLINT.
Left a comment in there to explain
With this fixed, SimCore compiles even with very aggressive warning settings (including warnings for narrowing conversions) with both GCC and Clang. With that done... I realized that there were only a couple of unformatted files left so I went ahead and formatted those too. I've kept the commits relatively well separated so it should be possible to check the individual parts despite the number of files touched.
This PR resolves #49 by introducing a new producer called a
ReSimulator
. This picks up the event seed from the event header of your input file and re-runs Geant4 with this seed. By default, all events in all of the input files will be resimulated. Alternatively, only a select number of events are resimulated with event numbers provided by the configuration. The resimulator is configured based on a simulator so unless you are trying, you shouldn't get any accidental mismatches.To do this, I've refactored the parts that are common between the ReSimulator and the regular Simulator producers into an ABC (SimulatorBase unless someone has a better name).
I've tested that the events that are produced by the ReSimulator are identical with a small analyzer that runs over all the simhits and simparticles and checks that they are the same between the collection from each).
Finally, this removes the
RootCompleteReSim
generator.