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

Stub builder fix and new SW #20492

Merged
merged 2 commits into from Sep 19, 2017
Merged

Conversation

sviret
Copy link
Contributor

@sviret sviret commented Sep 13, 2017

This PR contains an important bug fix for the Phase II tracker stub builder

@cmsbuild cmsbuild changed the base branch from CMSSW_9_4_X to master September 13, 2017 08:31
@cmsbuild
Copy link
Contributor

@sviret, CMSSW_9_4_X branch is closed for direct updates. cms-bot is going to move this PR to master branch.
In future, please use cmssw master branch to submit your changes.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@boudoul
Copy link
Contributor

boudoul commented Sep 13, 2017

@delaere FYI

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20492/667

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/PR-20492/667/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20492/667/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

@kpedro88
Copy link
Contributor

@sviret please apply code check patch as indicated

@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/PR-20492/693

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @sviret (Seb Viret) for master.

It involves the following packages:

L1Trigger/TrackTrigger

@cmsbuild, @rekovic, @mulhearn, @kpedro88 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @kreczko this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@kpedro88
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 14, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22978/console Started: 2017/09/14 17:36

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2646869
  • DQMHistoTests: Total failures: 312
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2646368
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@kpedro88
Copy link
Contributor

@sviret the comparisons show a general increase in the number of stubs for all eta. Please confirm this is the expected behavior.

stubs vs eta

@sviret
Copy link
Contributor Author

sviret commented Sep 19, 2017

@kpedro88, yes this is an expected behaviour, thanks for noting it. The new stub window tuning introduced is inducing an increase of the stub rates. The previous tuning was too sharp, so that efficiencies at low pt were not very good.

@rekovic
Copy link
Contributor

rekovic commented Sep 19, 2017

+1

@rekovic
Copy link
Contributor

rekovic commented Sep 19, 2017

@sviret
Can we have a backport of this to 93x, please.

@kpedro88
This would be needed for a MC production in 93x, which L1 Trigger is trying to piggy back on
HGCal MC production release.

@kpedro88
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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 (and backports should be raised in the release meeting by the corresponding L2)

@kpedro88
Copy link
Contributor

@rekovic the window for new changes in the initial production release closed last week. Maybe this can be put into a followup 93X release for L1-specific samples.

@davidlange6
Copy link
Contributor

+1

@smuzaffar
Copy link
Contributor

@sviret , @davidlange6 : looks like we have couple of relvals failing in IBs after these changes. Error is here

----- Begin Fatal Exception 20-Sep-2017 17:26:46 CEST-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 3 stream: 2
   [1] Running path 'L1TrackTrigger_step'
   [2] Calling method for module TTStubBuilder_Phase2TrackerDigi_/'TTStubsFromPhase2TrackerDigis'
Exception Message:
A std::exception was thrown.
vector::_M_range_check: __n (which is 13) >= this->size() (which is 13)
----- End Fatal Exception -------------------------------------------------

gdb show this

#0  __cxxabiv1::__cxa_throw (obj=obj@entry=0x7fff8a323420, tinfo=0x7ffff6449a78 <typeinfo for std::out_of_range>, dest=0x7ffff6379920 <std::out_of_range::~out_of_range()>)
    at ../../../../libstdc++-v3/libsupc++/eh_throw.cc:62
#1  0x00007ffff638ca7d in std::__throw_out_of_range_fmt (__fmt=__fmt@entry=0x7fffc1461368 "vector::_M_range_check: __n (which is %zu) >= this->size() (which is %zu)")
    at ../../../../../libstdc++-v3/src/c++11/functexcept.cc:104
#2  0x00007fffc145f6fb in std::vector<double, std::allocator<double> >::_M_range_check (__n=<optimized out>, this=<optimized out>)
    at /cvmfs/cms-ib.cern.ch/nweek-02490/slc6_amd64_gcc630/external/gcc/6.3.0/include/c++/6.3.0/bits/stl_vector.h:804
#3  std::vector<double, std::allocator<double> >::at (__n=<optimized out>, this=<optimized out>)
    at /cvmfs/cms-ib.cern.ch/nweek-02490/slc6_amd64_gcc630/external/gcc/6.3.0/include/c++/6.3.0/bits/stl_vector.h:843
#4  TTStubAlgorithm_official<edm::Ref<edm::DetSetVector<Phase2TrackerDigi>, Phase2TrackerDigi, edm::refhelper::FindForDetSetVector<Phase2TrackerDigi> > >::PatternHitCorrelation (this=0x7fffd271ee00,
    aConfirmation=<optimized out>, aDisplacement=<optimized out>, anOffset=<optimized out>, aTTStub=...)
    at /build/muz/gdb/CMSSW_9_4_X_2017-09-22-2300/src/L1Trigger/TrackTrigger/src/TTStubAlgorithm_official.cc:119
#5  0x00007fffc148f20c in TTStubBuilder<edm::Ref<edm::DetSetVector<Phase2TrackerDigi>, Phase2TrackerDigi, edm::refhelper::FindForDetSetVector<Phase2TrackerDigi> > >::produce (this=0x7fffc93fb680, iEvent=...,
    iSetup=...) at /build/muz/gdb/CMSSW_9_4_X_2017-09-22-2300/src/L1Trigger/TrackTrigger/plugins/TTStubBuilder.cc:104

@sviret
Copy link
Contributor Author

sviret commented Sep 23, 2017

Very strange error.... Is the tracker geometry of your relval T5, because this is the error appearing when the stub builder is used with obsolete t3 geometry

@boudoul
Copy link
Contributor

boudoul commented Sep 23, 2017

Hi @sviret , the track trigger group requested to keep for some time the flat Geometry, and therefore has been kept by the offline team for this request .
And if you look at the IB failure, it's indeed the 2023D20 scenario which is failing (running the flat geometry https://github.com/cms-sw/cmssw/tree/master/Configuration/Geometry ) .

@kpedro88
Copy link
Contributor

@boudoul is the flat geometry still needed?

If so, we have a couple of options:

  1. remove track trigger from the D20 workflows
  2. add separate Eras to customize the track trigger for the flat geometry (incurs some maintenance burden)

I would prefer option 1 if it's acceptable.

@sviret
Copy link
Contributor Author

sviret commented Sep 25, 2017

@boudoul, I tend to support @kpedro88 fully on that. Keeping flat geometry in the track trigger is purely useless. In any case the stub builder is not maintained in that view anymore, and I heavily suspect that the track trigger reconstruction codes are not either.

@smuzaffar
Copy link
Contributor

@boudoul , @sviret any update/progress on this?

@boudoul
Copy link
Contributor

boudoul commented Sep 27, 2017

Hi @smuzaffar , gimme <= a day (giving a talk at the CMS Week today) will come back to you after

@boudoul
Copy link
Contributor

boudoul commented Sep 28, 2017

Addressed here : #20687
and backported for 93X here too : #20688
thanks.

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

7 participants