FlagBkg clustering hit fix #660
Conversation
|
Hi @bechenard,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for 404cc08: build (Build queue is empty) |
|
☀️ The tests passed at 404cc08.
N.B. These results were obtained from a build of this Pull Request at 404cc08 after being merged into the base branch at bea231a. For more information, please check the job page here. |
|
Bertrand: Any suggestions for appropriate reviewers? |
|
📝 The HEAD of |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 404cc08: build (Build queue has 1 jobs) |
|
I did a lot of cleanup, and the real changes are minimal. Anyone who can read code could do it.
…Sent from my iPhone
On Nov 18, 2021, at 11:26 AM, Rob Kutschke ***@***.***> wrote:
Bertrand: Any suggestions for appropriate reviewers?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
I had a quick look. My impression is that "cleanup" means just removal of obsolete code. Is that right? Do you expect changes to nightly validation. I don't think that we make any Offspill digs during nightly validation. |
|
☀️ The tests passed at 404cc08.
N.B. These results were obtained from a build of this Pull Request at 404cc08 after being merged into the base branch at 6725199. For more information, please check the job page here. |
|
What tests have you done to check the behaviour of this change? I can see that it should work well for reco.fcl since the events are mixed and I bet that maxtime is stable to within a few ns. I can see possible pathologies with both ceSimReco and with reco of offline simulated offline events; when the only hits in the event are from track of interest, track will be pegged to maxtime - is that an issue? |
|
Not sure I understand the question. How get a bunch of hits with
different times, and I just need to find the max time to build the
proper data structure for the algorithm. It is on an event by event
basis, so this shouldn't impact things downstream. The only difference
is that the definition of the bin width in that structure is now allowed
to be a bit smaller than it was before, which means a few edge effects
for the clustering algorithm.
I tested the old/new version with reprocessing Validation/reco.fcl on a
digi file provided from Dave Brown (latest version). I also tested with
the VST file that Ray provided me.
Not sure I understand your last sentence :-)
…On 11/18/2021 1:39 PM, Rob Kutschke wrote:
What tests have you done to check the behaviour of this change? I can
see that it should work well for reco.fcl since the events are mixed
and I bet that maxtime is stable to within a few ns. I can see
possible pathologies with both ceSimReco and with reco of offline
simulated offline events; when the only hits in the event are from
track of interest, track will be pegged to maxtime - is that an issue?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#660 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJ27JDQPCUO7CKCTGOUI6TUMVXABANCNFSM5IIOCROQ>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Thanks Bertrand. Please also try ceSimReco - probably 1k or 2k events is fine. |
|
Could you send me the command, I forgot what it is...
Bertrand
…On 11/18/2021 2:42 PM, Rob Kutschke wrote:
Thanks Bertrand. Please also try ceSimReco - probably 1k or 2k events
is fine.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#660 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJ27JFLC6IWUQXUSMX754TUMV6OBANCNFSM5IIOCROQ>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
mu2e -c Production/Validation/ceSimReco.fcl -n 2000 |
There was a problem hiding this comment.
This looks good to me. It makes sense to use the hits themselves to define the time window, that minimizes the places where we need to explicitly keep track of on/offspill and such. I'm glad that you're pruning out obsolete methods. We will need to generate some OffSpill MDC data before this can be fully tested.
|
Hi Bertand,
* Not sure I understand your last sentence :-)
Taking this Offline. First checking something – this is the code that looks for clusters of hits in time, azimuth and radius, right?
Normally a time cluster has a low level of hits on both the low time side and the high time side. If the cluster ends at the edge of the binning, there is no low level of hits on the high side. Is the algorithm robust against that? I bet that in ceSimReco that will happen a lot.
Rob
|
|
I'm glad you are glad!
…On 11/18/2021 3:17 PM, David Brown wrote:
***@***.**** approved this pull request.
This looks good to me. It makes sense to use the hits themselves to
define the time window, that minimizes the places where we need to
explicitly keep track of on/offspill and such. I'm glad that you're
pruning out obsolete methods. We will need to generate some OffSpill
MDC data before this can be fully tested.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#660 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJ27JE73QRAPQVW2DO5ELDUMWCQVANCNFSM5IIOCROQ>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
📝 The HEAD of |
There was a problem hiding this comment.
I accidentally got Bertrand and myself into an offline thread. He ran valCompare on the output of ceSimReco and reports:
"Slight changes in all plots but the results are globally compatible. this is what I would expect if i tweaked the hit clustering algo parameters."
Thanks for checking this for me.
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 404cc08: build (Build queue is empty) |
|
☀️ The tests passed at 404cc08.
N.B. These results were obtained from a build of this Pull Request at 404cc08 after being merged into the base branch at a65549b. For more information, please check the job page here. |
A fix for the bug Richie encountered with the FlagBkgHit module, making the module independent of the "microbunch length". I took the opportunity to do some cleaning as well. Check that the track reco performance is essentially unchanged (there is a small change in the hit flagging algo but the impact is very limited)