Fixes to LFHCal simulation digi/reco #388
Conversation
| m_dyRangeADC=100. * dd4hep::MeV;//{this, "dynamicRangeADC", 100. * dd4hep::MeV}; | ||
| m_pedMeanADC=400;//{this, "pedestalMean", 400}; | ||
| m_capADC=4096;//{this, "capacityADC", 8096}; | ||
| m_dyRangeADC=200.;//{this, "dynamicRangeADC", 100. * dd4hep::MeV}; |
| m_pedMeanADC = 20; | ||
| m_pedSigmaADC = 0.8; | ||
| m_capADC = 4096; | ||
| m_dyRangeADC = 200. ; |
There was a problem hiding this comment.
Please ensure units are used here
There was a problem hiding this comment.
Hi - I had to remove the units since dyRangeADC secretly adds them in CalorimeterHitReco.cc in L27 like this:
dyRangeADC = m_dyRangeADC * dd4hep::MeV;
So If I presented it with MeV, then it would be scaled twice (which completely threw me off). I would prefer dyRangeADC to be assumed to have energy units, and remove this hidden rescaling (unless I missed something).
There was a problem hiding this comment.
Hidden conversions are bad. Let me look into that. The problem is that we have issue with units. @wdconinc started to fix those and filter out what happens in calorimetry algorithms with units here: #365. But it has not yet been finished. And I started to put the correct default values from python to C++. With all that we decided that the correct implicit units shold be DD4Hep ones. @wdconinc is it known and fixed in your PR?
There was a problem hiding this comment.
#365 does indeed remove the explicit addition of MeV in CalorimeterHitReco.cc, https://github.com/eic/EICrecon/pull/365/files#diff-7a64db59fabe7d6949bed432036c5f5250a349e7e4d50fc7d4d20a39a96c6b36L27.
|
Hi - just to clarify: i'm waiting for #365, so I can test it properly? |
|
@steinber Lets commit it as is (as it works in main now) and then probably it would be a critical place to watch for error to reappear. I hope on your help here and validation of what is being done. |
|
Thanks - i'm not remotely done with this, so I'm happy to keep up with it. |
Briefly, what does this PR introduce?
This includes some changes (hard won) which fixes inconsistencies in the digi and reco steps in the LFHCal (previously they were far out of sync, and so many of the energy deposits were not being reconstructed)
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
Nothing broke before, but most of the energy deposits were truncated and so no clusters were dfound
Does this PR change default behavior?
The LFHCal reconstructs now reconstructs energies (although the scale has changed since 2 weeks ago - under investigation).