-
Notifications
You must be signed in to change notification settings - Fork 484
ITS - (and MFT), better TF sampling in the deadmap builder #13349
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
Conversation
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
|
Thanks a lot @nicolovalle, sounds good to me! I do not expect a dramatic increase, right? Otherwise you should count the TFs arriving at the aggregator (that you are doing with |
Hi @iravasen , thanks for the comment. Basing the criterion on the number of arrived TFs rather than the orbit number indeed has a disadvantage: it increases the likelihood of larger gaps between map snapshots (gaps = in human time: seconds or orbit number) . The logs demonstrate that TFs arrive at the aggregator in a random order. As a result, using the condition Actually your comment prompted me to study the numbers a bit better... From the online logs, I see that if the n-th TF arriving at calib0 contains orbit X, then the (n+1)-th TF will contain an orbit approximately in the range [X-100k, X+100k]. It is very large... also my current proposal is ineffective! |
|
Thanks a lot for the clarification @nicolovalle! Ok yes, once I'm back from vacation let me know if you need help with some checks or thinking :-) |
|
The improved sampling is now in place. A new configurable (tolerance) is introduced. The sampling condition returns true if the TF index (calculated as orbit/TF_length) falls within any interval [k * tf_sampling, k * tf_sampling + tolerance) for some integer k, provided no other TFs have been found in the same interval. This requires additional operations with containers. The code has been tested, and with default values (suitable for the online), the processing time for the accepted TFs (< 5% of the total) remains below 1 ms, comfortably within acceptable limits for online. (I would merge this, if you agree @iravasen -no need to reply soon :)- ) |
|
Thanks a lot @nicolovalle, sounds good to me! I approved it. I let @mconcas / @shahor02 merging it. |
@iravasen , @jovankaas, @IsakovAD, below the details of the fine tuning in this PR:
**outdated (see conversation below): **
The dead map builder is currently sampling online 1 TF every 350 using the condition "CurrentTF % 350 == 0".
I've noticed that few hiccups may occur resulting in gaps of 2x, 3x or 4x 350 TFs.
This PR tries to workaround this, by modifying the sampling condition as follows: "CurrentTF % 350 == 0 OR CurrentTF - LastProcessedTF > 350".
caveat: depending on the order of arrival of the TFs to the aggregator node, this may result in oversampling, good from the physics point of view but increasing the size of the object handled by the workflow.
** New implementation in the second commit **, commented below