LDMX-Software / SimCore

Integration of Geant4 simulation framework into a more generalized event processing framework.
2 stars 0 forks source link

Hcal SimHit updates #72

Closed EinarElen closed 2 years ago

EinarElen commented 2 years ago

We need to record some additional information in the SimHits for https://github.com/LDMX-Software/Hcal/issues/12 by @ehrlich-uva. In particular Ralf needs

For SimHits with just one contrib, like what we do in Hcal, this would be straight-forward to add to just record as part of the SimCalorimeterHit. I don't think recording all of this for individual contributions would make sense for

I'm not all that familiar with what happens underneath the hood with ROOT, does this revision require us to bump the ClassDef number?

@ehrlich-uva what units do you want these quantities in?

tomeichlersmith commented 2 years ago

does this revision require us to bump the ClassDef number?

Yes, any change to any part of the class requires changing the ClassDef number. ROOT's dictionary generation almost copies the full class structure into the dictionary library file, so you need to let ROOT know you are changing things on purpose.

Velocity

Leave it as a signal value of -1. This should never be used in ECal even in digi emulation.

Larger Redesign Discussion

Please read LDMX-Software/ldmx-sw#1317 as background (lol).

Only Ecal uses the contrib machinery and it is confusing at best. The contrib machinery was implemented as a first attempt to have the cake and eat it too - reducing the number of hits in the output file (thus saving space) and keeping a "maximum" amount of truth information. As I imply with my "cake" comment, I don't think this is possible and as discussed in LDMX-Software/ldmx-sw#1317 , I believe a better way forward would be to take the more transparent approach of being clear with the user about what hits are merged (or split) and what hits aren't. Now that we have fully-configurable SDs (:tada:), the merge/split machinery can be entirely held within EcalSD and does not need to "bleed" into SimCalHit. All of this comes to a more drastic proposal: delete the contrib machinery. This would save space on disk providing room for the extra 10 floats necessary for HCal and open the door to resolve LDMX-Software/ldmx-sw#1317 . We could even do a "soft" deletion where the contrib-style accessors are still there, but they only ever provide a single contrib.

Downstream Effects

I would like to ask @bryngemark to weigh in on this as well since TS is the other subsystem pipeline that starts with SimCalHits.

EinarElen commented 2 years ago

I can attest to that the contrib portion of the hits has confused more than one student at this point.

bryngemark commented 2 years ago

We also don't use contribs in the TS, meaning we know of them only in the Hcal fashion with single-contrib hits.

Dropping contribs would also clean up the overlay procedure (as you allude to @tomeichlersmith), which could then be based on simhits for all collections and not make an exception for Ecal the way it does right now. Then the only remaining system specific treatment there is between tracker and calo hits.

tomeichlersmith commented 2 years ago

Alright, then the only thing we have to worry about is backwards-compatibility. How can we remove the contrib machinery while supporting the ability to read previously-generated files?

I'm not confident on how to handle this without some experimentation. Hopefully ROOT allows reading while dropping some of the members? If not, we may want to have a "legacy" and "new" SimCalHit class so that the "legacy" SimCalHits are still accessible by folks analyzing old files. (I'm thinking "legacy" would just be ldmx::SimCalHit and the "new" class would be simcore::event::SimCalHit but only changing the namespace may be confusing.)

The other option is to explicitly break backwards compatibility. We've been burned by this in the past, but that burn may hurt less if we are better at communicating it?

bryngemark commented 2 years ago

I think communication is key: surveying what legacy files people are using and what aspects of them, and then deciding how much backwards compatibility is needed. For the Ecal I think the UCSB folks are the main people besides you @tomeichlersmith who might care about contribs? This might actually also be a good time to think about if we handle scoring planes in a satisfactory manner when it comes to overlay. --> probably a separate issue.

We do have some v2.3 --> v3.0 rereco samples which I fear are anyway becoming hard to work with for reasons I haven't been able to understand while debugging. Given the new campaign coming up, and no paper being based on those samples afaik, making them obsolete might not be that big of a big deal.

EinarElen commented 2 years ago

It sounds to me like there is some discussion that we want to have about this and how to do it right. For the purpose of the phase 1 validation production, I'm suspecting that the best way would be to do as little as possible. If the Ecal is the only part that deals with contribs and the Ecal is uninterested in these quantities, maybe it would be best to just keep these things defaulted (e.g. -1 for velocity) for now?

For TS, I could record the same things or I could leave them unused. Do you have a preference @bryngemark?

bryngemark commented 2 years ago

I don't think we'll concern ourselves with cherenkov light so velocity can be left out for us, but the rest is interesting, insofar as it can have a practical use for the Birks' law calculation. But I take it that this hasn't been fully ironed out yet. So... I prefer to have them set?

EinarElen commented 2 years ago

Ok, if it isn't adding things that you don't need then I can go ahead and implement the corresponding part in the TS SDs.