-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add Hybrid Format SiStrip Zero Suppression algorithm for 2018 PbPb data-taking #24339
Add Hybrid Format SiStrip Zero Suppression algorithm for 2018 PbPb data-taking #24339
Conversation
- parse input tag list in constructor (instead of produce) - use move to reduce the number of copies - auto and range-based for loop for readability - 'mergeCollections' option had almost completely non-overlapping code, so became a new EDProducer SiStripMergeZeroSuppression
e.g. const uint32_t& -> uint32_t. Also some whitespace cleanup
16 and 32-bit integers by value, inspect methods take digis by const reference
- inspection method for Hybrid emulation selected through config (no separate method), also APV flags vector is reused - removed templating on digi integer type, it's always int16_t - more const (+ followup changes in other places) - for-each, auto, typedefs and other simplifications - reorder options
Comparison job queued. |
On Wed, 5 Sep 2018, 23:34 Slava Krutelyov, ***@***.***> wrote:
Is this SiStripAPVRestorer::cleaner_LocalMinimumAdder contributing to
the digi step
Yes (it is called for APVs where the BaselineFollower finds a nonflat
baseline)
If this is addressing the regression in the old/classic ZS (what's
called by the siStripZeroSuppression module), then perhaps one really
has to run this VR -> ZS repacking step a la HLT for HI. This step is
not present in any of the existing relval workflows. This could explain
why it escaped our attention until I was remaking this step for this PR
tests.
It should indeed be the cause of those differences.
The only realistic way to test this code path in the relvals is by running
the ZS on VR (or hybrid) HI data (nonflat baselines are negligibly rare in
pp), so I agree it's a good idea to add this step (but we should also have
checked this specifically while preparing the code changes).
|
Comparison is ready Comparison Summary:
|
Addressing a point raised by @slava77 in an email thread about the HI relvals: with the latest version of the code the HLT and zerosuppression+repacking can be run in the same step, but with some workarounds for the fact that the 2015 HI files have already been repacked in the HLT before.
(where dropping the previous HLT products in step1 and @Martin-Grunewald what would be the proper way to have the zs+repacking done in the HLT? What would be needed is essentially |
+1
|
+operations changes to the StandardSequences look coherent with the purpose of the PR |
@Martin-Grunewald do you still have pending questions/comments that are not addressed? Or can we move forward with this PR? I see a couple of questions of 18 days ago that were not explicitly answered by @CesarBernardes if I am not mistaken |
He confirmed that the HLT settings reproduce the old behaviour. |
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
Introduces Hybrid Format SiStrip Zero Suppresion for HI 2018 data-taking.
The code has the double utility of:
Emulates hybrid format: it takes as input VirginRaw data and it coverts to Zero Suppressed 10 bits hybrid format as expected from the tracker FEDs.
Performs hybrid data zero suppression: takes Zero Suppressed 10 bits hybrid format and converts it to 8 bits zero suppressed data
@mmusich @echabert @pieterdavid @icali @abaty @mandrenguyen