-
Notifications
You must be signed in to change notification settings - Fork 32
fix the weight calculation in raw to sim hits association #2065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The weight was set to 1 manually regardless of how many sim hits on the same cell. This update should provide a rough but more correct weight.
for more information, see https://pre-commit.ci
hitassoc.setWeight(1.0); | ||
double weight = 0.0; | ||
if (raw_hit.getCharge() > 0) { | ||
weight = std::round((sim_hit.getEDep() * 1e6 / raw_hit.getCharge()) * 10.0) / 10.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these magic numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why the rounding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by "magic number" I guess you mean the times 10 for rounding?
In line 72 will fill the charge with (std::int32_t)std::llround(sim_hit.getEDep() * 1e6). one could use exactly this formula. I wanted to use rounding to make sure it will return 1 instead of 0.99999 for example, and I assume we don't care about too much about precision. Would times 100 (two decimal digits) be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't round.
Other magical number is the 1e6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1e6 is the unit conversion. Can you suggest some alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I changed to use eV for rawhit.charge so it's always integer. It was keV with the magic number of 1e6, which is not the best choice because our detector threshold is often <1keV.
We can have a separate discussion later on whether it makes sense to declare RawTrackerHit.charge as int32_t.
for more information, see https://pre-commit.ci
I don't know how we've come this far without proper energy deposition digitization in the silicon tracker. What bit depth and dynamic range do we expect there? This digitization algorithm can learn a lot from the calorimeter digitization... It almost doesn't make sense to tack on more hacky hard coded bit depth and dynamic range assumptions with this PR until the underlying logic is fixed. |
I thought we'd decided to leave this in place, rather than spend time patching it, while pursuing proper technology dependant approaches.
Is that not the quality flag for |
Edited: the quality value I saw is 1073741824, which is indeed the secondary bit (30th) given how the set_bit function works. Thanks for pointing it out. Both hits are associated with the same particle, mainly from SR. |
I will follow up with you and Simon to arrange a dedicated discussion on edep digitization scheme. Any more suggestions on the current change? |
Briefly, what does this PR introduce?
In Si digi, the weight between raw and sim hit was set to 1 manually regardless of how many sim hits on the same cell. This update should provide a rough but more correct weight.
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?
Does this PR change default behavior?