Iss49 Initial version of a ReSimulator

[EinarElen]

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.

[tomeichlersmith]

This is awesome 😍

I think this is great work and something I haven't been able to get working for awhile so I am very excited to see this introduction. Since this is a relatively big feature, I plan to do a general review first (this one) and then a more detailed review after further discussion and updates. I have two requests and a question.

Requests

1. Fill out documentation and align format. Maybe doxygen comments aren't needed for member variables (unless you can think of something interesting to say), but for member functions - especially the new ones you introduced - it would be nice to have a comment saying their intended purpose and where they should be used. By "align format", I mean look at running clang-format on the files that you've added/changed as well as move implementations of functions to the implementation file (unless they need to be in the header?).
2. Rework the python config. I think we could enforce the requirement that a resimulator is built from a simulator by having the resimulator class only built with a simulator method. I'm thinking something like

class simulator(Processor):
    # normal simulator stuff
    def resim(self, which_events = None):
        # copy simulator and "promote" it to a resimulator
        the_resim = self
        the_resim.className = 'simcore::ReSimulator'
        if which_events is None:
            the_resim.resimulate_all_events = True
            the_resim.events_to_resimulate = []
        elif isinstance(which_events, list):
            the_resim.resimulate_all_events = False
            the_resim.events_to_resimulate = which_events
        else:
            raise ValueError('which_events must be a list if provided')
        return the_resim
# delete resimulator class definition so this function is the only way to resim

Question

One of the main use cases of a resim is to get more detail about events that have been filtered out of a previous larger sample due to some special criteria. Have you checked that the resimulator properly resimulates only a subset of the events of an input file? I'm particularly worried that skipping events may break the resimulation. (If we are saving and loading the event seed properly, this should not be a problem but since I recall this being a problem before I wanted to double check).

[EinarElen]

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 :)

[EinarElen]

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 LDMX-Software/ldmx-sw#1176 and LDMX-Software/ldmx-sw#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

• Adding missing virtual destructors, override specifications etc. Note: All types that use the ROOT classdef macro have virtual functions
• Removed some commented out code
• Remove const from by-value return types (these do not actually do anything)
• Remove variables and parameters that aren't used directly (with one exception in RunManager) or mark them as [[maybe_unused]] if they have side-effects
• Use dynamic_cast for downcasts rather than static_cast
• Remove tautological check in BertiniEventTopologyProcess which always returned true (clang caught this, basically we were checking if a PDG code was larger than a number that was larger than intmax)
• Don't allow non-void functions to reach the end without return or exception (see GammaPhysics)
• Make trivial destructors and constructors defaulted
• Remove redundant else-clauses (e.g. if (foo) { return 3;} else { // stuff } -> if (foo) {return 3;} // stuff
• 0/NULL -> nullptr

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.

• We are currently leaking the 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
• In the SensitiveDetector library, we are defining two virtual functions called 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.

[EinarElen]

I also added some draft documentation over at
https://github.com/LDMX-Software/ldmx-software.github.io/blob/resimulator/docs/Generating-Simulation-Samples.md

[tomeichlersmith]

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.

[EinarElen]

I'll give storing it in the constructor a try!

Maybe OnEndOfEvent or OnFinishedEvent or something along those lines?

[EinarElen]

Moving it into a unique_ptr in the constructor worked like a charm :)

[EinarElen]

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.

[tomeichlersmith]

@EinarElen and I chatted on slack and agreed to get this in - the formatting and refactoring is muddying the PR and I think it is beneficial to drop them in trusting clang to do right by us. We can make subsequent patches as we find bugs.