rebdsim Spectra command doesn't work with csampler

[Stuart]

Hello everyone. I am trying to make a spectra plot for my cylindrical sampler. rebdsim crashes when I try.

‌

The offending line:

SpectraLog STERN_TUBE 100 {-3:4} {top10} 1

where STERN_TUBE is a csampler.

The identical line but for a normal sampler works fine (or at least, does not crash):

SpectraLog TUBE_START 100 {-3:4} {top10} 1

The error:

Event #       0 of 10000rebdsim(45574,0x1df015000) malloc: *** error for object 0x9999999900000000: pointer being freed was not allocated
rebdsim(45574,0x1df015000) malloc: *** set a breakpoint in malloc_error_break to debug

Random guess why:

for some reason in csamplers, the energy leaf is called totalEnergy instead of just energy. In the normal samplers it is called energy

(end of random guessing).

Anyway thanks. It’s generally very easy to use. Better than ever!

[Laurie Nevay]

I’ll have a look! Yes, I could imagine a bug here. I’ll check the testing as well.

[Laurie Nevay]

Confirmed it is a bug and you diagnosed it perfectly. I’m coming up with a fix. In rebdsim, we create all the expanded spectra command definitions before any data is loaded as it doesn’t depend on that and we can predict the variable names. Or so we thought. I made it “totalEnergy” for the new samplers as it was always historical it is “energy” in sampler instead of explicitly kinetic or total energy (it is in fact total energy). When making the new sampler classes, I made the variable more explicit but obviously didn’t think about spectra or test them with these samplers.

I tried adding a TTree alias (TTree->AddAlias()) but this doesn’t work as we don’t have splitting on by default for the samplers so it only sees the variables inside when it’s running.

I tried a LinkDef rule to map the variables but this only works for a new version of the data class versus an old version and here they are the same version.

Now, I will implement an ‘update’ to the variable names once we’ve loaded the data. I cannot predict the user-defined sampler names and only get these from the model data when loading a file. Only then do I know which ones are cylindrical and spherical samplers versus regular ones.

So, without major changes, the fix will be that I prepare the spectra histogram names as normal, then once the data is loaded, I got back and update their names for ones that apply to cylindrical or spherical samplers. A little untidy but it will have to be.

An alternative is to change the name of the sampler variable or totalEnergy → energy. Neither of which I really want to do. Maybe for v2.0 we could change the energy variable to totalEnergy.

The other spectra commands for momentum, kinetic energy or rigidity will work fine as the variables are the same. This is a temporary work around.

‌

[Stuart]

OK thanks for having a look into it. Just a couple of comments.

An alternative is to change the name of the sampler variable or totalEnergy → energy. Neither of which I really want to do. Maybe for v2.0 we could change the energy variable to totalEnergy.

I would say I was surprised that they weren’t the same. I wanted to copy paste my ana config lines and just change the sampler name, so it was some surprise to me get segfaults (or whatever the errors are) because of this issue. Also I think it’s quite clear what is meant by energy (personally) because of the presence of the kineticEnergy leaf meaning that it can only be total energy by process of elimination, so to me energy was already perfectly clear (Yes I know I have used BDSIM for years so I am biased :) ). Obviously not a big deal though, just my thoughts.

‌

An alternative is to change the name of the sampler variable or totalEnergy → energy. Neither of which I really want to do. Maybe for v2.0 we could change the energy variable to totalEnergy.

Actually I thought Spectra already meant kinetic energy plots. That’s why I was unsure of my initial guess because I believed I was plotting kineticEnergy, which is the same for both sampler classes, so I did not understand exactly how energy/totalEnergy came into it. E.g. in the documentation the table http://www.pp.rhul.ac.uk/bdsim/manual-develop/output_analysis.html#spectra-commands (see rows in bold) it says that Spectra=KineticEnergy plot:

‌

Command	Description
Spectra	Per-event histograms in kinetic energy
SpectraMomentum	Per-event histograms in momentum
SpectraTE	Per-event histograms in total energy
SpectraRigidity	Per-event histograms in rigidity
SimpleSpectra	Total histograms in kinetic energy
SimpleSpectraMomentum	Total histograms in momentum
SimpleSpectraTE	Total histograms in total energy
SimpleSpectraRigidity	Total histograms in rigidity

‌

[Laurie Nevay]

Yes, so I’ve fixed the variable name bug now. Indeed as you pointed out “Spectra” → kineticEnergy so the crash isn’t because of that actually. Still, if we had used “SpectraTE” it wouldn’t have worked.

There is actually a second bug where we assign the data to BDSOutputROOTEventSampler* from BDSOutputROOTEventSamplerC* which is a completely different class. This is because for the “top10” I need to manually (without TTree Draw) build up the set of PDG IDs in the data. So the PerEntryHistogramSet class explicitly uses the sampler pointer and looks at the partID data. Whilst the C and S samplers have this variable, they are of different underlying types that don’t inherit each other. So fundamentally, this was a separate bug. This causes the crash because we set both branch addresses to the file and we try to clean up at some point also. So, irrespective of variable names, this crash would have happened anyway.

“energy” as a variable is historic. I think it’s better to be explicit about whether it’s kineticEnergy or totalEnergy. Therefore, when creating the new C and S class, I was explicit about it. I’d need to check but I think the Spectra command was created after this and I don’t often use C and S samplers so it wasn’t thought of.

I also thought about making separate Spectra commands for C and S but I don’t like this solution either.

‌

[Stuart]

OK makes sense, sounds like there is some refactoring to do, then. But this feature is not so urgent for me, so don’t do it on my account :)

Thanks again.

[Laurie Nevay]

Ok, pretty sure I’ve fixed those two bugs now. It’s fixed in a branch called “sampler-c-fix-v1” for now. I’ve yet to make some more tests but once that’s done I’ll make a patch release. It’s good to fix as it really should never crash uncontrollably.

[Laurie Nevay]

Now fixed in develop. Even when the model isn't stored in the output file (and therefore we don't store the list of which sampler is which type), we can inspect the tree for the class name and link to the right local object for reading. The slightly more modern variable names in csampler and ssampler are also substituted correctly if the spectra is made on a csampler of ssampler.

In develop now and will be in the next release.