Skip to content
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

HFStripFilter for Run 2 data #26251

Merged
merged 19 commits into from Apr 5, 2019
Merged

Conversation

toropin
Copy link
Contributor

@toropin toropin commented Mar 26, 2019

PR description:

This PR is devoted to the development of a new filter HFStripFilter
for HF RecHits. This work was started when the JetMET group found a
lot of FakJets originated in HF. A careful examination revealed that
the high-energy hits from these jets have times ~4-6 ns and Ieta
mostly 29-34. This indicated that these hits could be the residual
tails after TDC cuts with QIE10. In Ieta 29-34 the distribution of
signals in time corresponding to early hits caused by possible PMT
hits are not well separated from the main physical signals and hits
with ~4-6 ns are probably due to tails from PMT hits (See slide 13
in ref. 1).
These early hits may be due to muons passing through "jungle" of
optical fibers coming from the HF absorbers to PMTs. In this case, we
can have a sequence of hits in one HF wedge, which can have signals in
both depths 1 and 2 and have a strip-like structure: sequence of hits
in one wedge and without or with very small signals in adjacent
wedges. Such signals cannot be marked with other HF filters, because
signals basically have comparable hits in both depths 1 and 2 and PET
and S9S1 HF filters work with hits which have very large signal in one
of depths and very small signal or no signal in another depth.
So HFStripFilter is looking for strips in HFRecHitCollection and marks
hits in the strips if they were found with special flag which could be
used for further data processing (See first part of report in ref. 2).

References:

  1. https://indico.cern.ch/event/800247/contributions/3325296/attachments/1800754/2937107/HFStripFilter_22Feb2019.pdf
  2. https://indico.cern.ch/event/807827/contributions/3362430/attachments/1816769/2969579/HFStripFilter_22Mar2019.pdf

PR validation:

The logs produced by "runTheMatrix" script look reasonable. There are few errors in log files, but one error related to data server eoscms.cern.ch which appears to be down or unreachable (cmsdev31.cern.ch). Some other workflows with errors have DAS_ERROR status which is also unrelated to this PR.

In attached Screenshots you can see pictures of EvenrDisplay (FireWorks) with HF rechits after reconstruction procedure. In column 'flags' you can see the upper 2 hits in table belonging to strip have values 1048640 (set bits 6, 20). Bit6 is new flag HcalPhase1FlagLabels::HFAnomalousHit).
Screenshot 2019-03-25 at 17 21 55
Screenshot 2019-03-25 at 17 18 58

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26251/8909

  • This PR adds an extra 40KB to repository

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26251/8909/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26251/8909/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26251/8917

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @toropin for master.

It involves the following packages:

DataFormats/METReco
RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @ahinzmann, @rappoccio, @mmarionncern, @rovere, @argiro, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26251/9069

  • This PR adds an extra 92KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

Pull request #26251 was updated. @cmsbuild, @perrotta, @slava77 can you please check and sign again.

@perrotta
Copy link
Contributor

perrotta commented Apr 4, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33955/console Started: 2019/04/04 12:30

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2019

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26251/33955/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3139747
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3139549
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Apr 5, 2019

Thank you @toropin @igv4321 @abdoulline for your updates and explanations.

Ok, now it is clear to me why there can be extra HF recHits if there is a status bit newly set.
What I still don't fully understand is why the location in which those additional recHits appear changed during the different tests made for this PR (at the beginning it was in wf 25020 and later it moved to wf 10224).

It could be that the input events changed already in the baseline, and therefore it can be that the filter acting on different events give some different result. Unfortunately, the matrix logs of the IB used for the first test got erased in the meanwhile, and this cannot be checked; the changes in the next two tests reproduce as expected, instead.

Just to be sure that nothing unwanted got included during the code updates that followed the review, could you please re-evaluate the performance with the latest version of this PR and confirm that they are still as expected?

@toropin
Copy link
Contributor Author

toropin commented Apr 5, 2019

Actually after each new 'git push ...' I made full check using data file I have in my directory (events selected by JetMET group). These events are enriched with strip-like cases.
Now I did this check one more time. Everything looks the same as it was at the beginning of this PR.
So I can say the performance has not changed.

@perrotta
Copy link
Contributor

perrotta commented Apr 5, 2019

+1

  • The status bit for the new HFStripFilter is set, and made available for possible evaluation
  • Extra HF reduced recHits can be saved if there is such a status bit newly set: this happens at a succesfully low rate that the increase in the event size is negligible
  • Also the cpu time taken by this filter is below the resolution of the measurement errors, as verified with wf 11024
  • Jenkins tests pass and the only differences reported are about those very few possible extra hfReducedRecHits

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2019

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)

@fabiocos
Copy link
Contributor

fabiocos commented Apr 5, 2019

+1

@cmsbuild cmsbuild merged commit fb478a3 into cms-sw:master Apr 5, 2019
@toropin toropin deleted the my-hf-strip-filter branch April 7, 2019 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants