Griffin crystal 0 histograms not filled

[VinzenzBildstein]

It looks like the histograms for griffin crystals 0 are not being filled. The information is in the tree, just the histograms aren't filled.

Is this related to #31?

[VinzenzBildstein]

As of #43 all histograms have been removed so the issue is moot.

[jsmallcombe]

Why were histograms completely removed? SPICE uses them. In fact we just spent a lot of manpower getting the working and right. For a lot of things the add step of parsing the tree afterwards is unnecessary extra step.

[VinzenzBildstein]

The whole idea is to run the simulations once, store the information w/o any artificial smearing applied, and then run the simulation files through NTuple or NTuple2EventTree, applying the desired resolution. This way changing the resolution is a cheap/fast process that doesn't require re-running the simulations. It has been that way for a while, the histograms in the simulation were just a leftover from when Evan started working on this. That is also why they were in such a bad state, I believe.

[jsmallcombe]

Sure for that example (and others) there are plenty of reasons to have/use the NTuple. But that's not a reason to completely remove the histograms, which can do a perfectly valid job for certain tasks.

[VinzenzBildstein]

Except for the fact that creating and filling the histograms slows everybody else's simulations down (and cluttering up the code, but that's a minor point and probably more my problem). I simply don't see why we need to slow everyones simulation down instead of just using the existing NTuple program (or NTuple2EventTree plus grsiproof).

[VinzenzBildstein]

Besides, the histograms did not do a perfectly valid job, some of them weren't filled (see the start of this issue).

[jsmallcombe]

​I​ am surprised if filling a few histograms noticeably slows down the simulation.

In the exact example you gave NTuples are faster. In many other cases it is just an extra step and the NTuples are slower, not the histograms. And the NTuples don't contain all the information I need.

We spent a lot of time getting the SPICE simulation working properly in the collaboration simulation, so that the collaboration would have them and they would stay maintained. And this totally breaks them. I don't see why I should have to recreate that work in a different format.

[VinzenzBildstein]

The filling does slow it down, but also the creation of the histograms, especially since they have to be created for each thread individually.

I've made a new branch "spice" which points back to commit c4d9aec (before the histograms were removed). This way you can run your simulations with spice histograms.

I understand that you put a lot of work into creating the right histograms in the simulation, I just don't understand why you decided yo put them into the simulation in the first place. The collaboration had the discussion about how to do this a while ago and decided that saving the un-smeared information is the best way.

[VinzenzBildstein]

After thinking about this over the weekend, I tried getting the spice histograms back in, but that is not so simple. Since you are now filling histograms not only from the EventAction, but also the PrimaryGeneratorAction, this now also needs access to the HistoManager. I think I got everything back in. Though the histograms will only be created if spice is used. I've also cleaned up the code at the same point, making all class members private, and removing unused code.

I still have some tests to do before I can commit the code, but I will do that soon.

[VinzenzBildstein]

Merged as of PR #46.

I've tested with GRIFFIN and SCEPTAR, and the result is still the same (no histograms). Haven't tested the SPICE histograms (since I have no idea how they should look).

[VinzenzBildstein]

I'm closing this issue for now. If needed we can open it again.

[jsmallcombe]

Thank you Vinzenz. Other than the reasons given above, it was done that way due to inheritance from my predecessors and not getting the memo earlier. I'll test the latest version when we finish the current simulation work we are doing. If you are ok with us adding some PrimaryGenerator information into the ntuple I will try and have someone change SPICE over down the line.

[VinzenzBildstein]

I think adding information from the primary generator to the ntuple is a good idea, maybe we can make it so that we can select what is written to the ntuple via a messenger. That way we can select in the macros how much is written. Let me know if there are any issues with the latest version (you need to use the geant4.10.04 branch for that). If you want to do the test with an older version first, I could try and get the changes over into the master branch as well.

[jsmallcombe]

I got geant4.10.04 working and tried the branch, running the same script as before. Both histograms and ntuple are empty. Could well be something obvious I'll find on Monday, I'm just tinkering remotely at the moment.

[VinzenzBildstein]

Can you send me the script you're running? I could try and run it here.

[jsmallcombe]

Have emailed as it didnt want to upload here. I had a couple of un-pushed changes on the other branch, but I had a brief check and I dont think thats it. Even the primary generator action histograms aren't filled, which should be done even if there was some problem with the detector or physics or stepping action code. I'll haven't done any proper in-code debugging myself yet to see where the problem is yet.

[VinzenzBildstein]

Okay, I've ran the simulation (for some reason I had to comment out the line setting the world material to vacuum, I'll check what the problem there is).

At first it would crash at the very end, after complaining that no master instance of G4RootAnalysisManager exists. This was because I had changed the ActionInitialization function BuildForMaster to not create a HistoManager as that only created an empty g4out.root file w/o any entries in the ntuple.

After putting the HistoManager back into the BuildForMaster function, I do now get histograms in the g4out.root file (still no entries in the ntuple tree though). Only two of them have entries: astats_particle_type_in_each_event - 10000 entries, all in [0,1) BeamEnergy - 1000 entries in a single bin around 2 MeV - fAngleDistro[0]

all others are empty: astats_particle_type_in_each_step - empty FullEdep - empty - fSpiceHistNumbers[0] Edep_Addback - empty - fSpiceHistNumbers[1] z-distro - empty - fAngleDistro[3] AllSegEnergies - empty - fAngleDistro[1] x-y - empty - fAngleDistro [2] AngRS - all empty - fSpiceAngleHists[]

The other root file that was created was g4out_t0.root (I was running with a single thread), and had only an empty ntuple tree in it.

[VinzenzBildstein]

I tried putting a smart statement into the BuildForMaster function to only create the HistoManager if we use SPICE, but this happens at a point where the detectors haven't been built yet, so the detector construction can't tell us that SPICE is being used. BTW, the empty ntuple was intentional, as this isn't filled if we use SPICE (since the histograms are filled in this case). If you want to have both filled, we can change that easily.

[VinzenzBildstein]

So we do fill one of the AngleDistro histograms (0 - BeamEnergy), but not the others. In the code we currently define AngleDistro histograms: 0 - BeamEnergy 1 - AllSegEnergies 2 - x-y 3 - z-distro But it seems we only ever attempt to fill the AngleDistro histograms 0 - BeamEnergy 1 - AllSegEnergies 3 - z-distro 4 - undefined !!! (in PrimaryGeneratorAction) 8 - undefined !!! (in EventAction if SpiceRes is used)

AngleDistro's 3,4,0 are filled in one single if-statement inside PrimaryGeneratorAction. Based on the variables they are being filled with, I guess the indices are wrong and should be 2,3,0. Changing this I do get entries in histograms z-distro (10000 at [0,0.2)), and x-y (10000 in a blob around 0,0).

The other histograms aren't filled it seems, because the energy in spice is always zero.

[VinzenzBildstein]

Looking at the output with /tracking/verbose 1, it seems the electrons all kinds of volumes, but never a spice detector. Since I'm not familiar with the geometry I'll let you figure that out. I've pushed the changes I've made (changing the indices used for AngleDistro is the only important one).

[jsmallcombe]

Thanks for looking at that Vinzenz. I've merged the changes I have with that branch and the histograms seems to working as expected now.

[VinzenzBildstein]

Great, I'm glad we could solve this issue.