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

Proper use of eras for L1Reco and a fastSim fix #16118

Merged
merged 2 commits into from Oct 8, 2016

Conversation

Dr15Jones
Copy link
Contributor

The tests in CMSSW_8_1_DEVEL_X are failing because of an unrunnable schedule when using fastSim with the legacy L1 trigger. Although it would have been possible to fix just that problem, the L1TReco_cff file was not using the modern recommended practice for eras. Therefore I also updated the use of eras.

Moved the era modifications of l1extraParticles from
L1Trigger.Configuration.L1TReco_cff to here where they are meant
to be.
The default parameters for l1extraParticles now match the legacy trigger
values used in L1TReco_cff.
The 'isChosen' interface is deprecated for Modifiers so needed to
be replaced with with modern recommended usage for eras.

The module l1L1GtObjectMap never worked properly when run in fastSim
since it needed data from a module which was run in a later Path.
The result was the data from l1L1GtObjectMap was never put into the
event for fastSim. Now that the framework is going to run Paths
in parallel, the improper data dependency is producing an unrunnable
schedule. Therefore remove the module from the Sequence gives
functionally equivalent behavior as before this change.
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2016

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_8_1_X.

It involves the following packages:

L1Trigger/Configuration
L1Trigger/L1ExtraFromDigis

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

cms-bot commands are list here #13028

@Dr15Jones
Copy link
Contributor Author

I used the following program to test that my changes give functionally equivalent configurations

import sys
import FWCore.ParameterSet.Config as cms

from Configuration.Eras.Modifier_stage1L1Trigger_cff import stage1L1Trigger
from Configuration.Eras.Modifier_stage2L1Trigger_cff import stage2L1Trigger
from Configuration.Eras.Modifier_fastSim_cff import fastSim

modifiers = []
for i in sys.argv[1:]:
    print "***********"
    print i
    print "***********"
    if i == "fastSim":
        modifiers.append(fastSim)
    if i == "stage1":
        modifiers.append(stage1L1Trigger)
    if i == "stage2":
        modifiers.append(stage2L1Trigger)


process = cms.Process("Test",*modifiers)
process.load("L1Trigger.Configuration.L1TReco_cff")

print process.dumpPython()

I applied this to both the original version of the code and this pull request for the following combinations of era

  1. no eras
  2. just fastSim
  3. stage1
  4. stage1 and fastSim
  5. stage2
  6. stage2 and fastSim

Doing a diff on the output showed that the change I made gave functionally equivalent configurations. By 'functinally equivalent' I mean

  1. the new configuration may contain unuse module declarations and additional empty sequences
  2. the change when running fastSim with legacy where l1L1GtObjectMap is removed

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15573/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2016

-1

Tested at: c40cda6

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16118/15573/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2016

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15584/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2016

@Dr15Jones
Copy link
Contributor Author

Ping
This is needed for the next phase of the threaded framework.

@Dr15Jones
Copy link
Contributor Author

I looked through all the comparison plots and the only one with even a minor difference was

25202.0 HLT >> Muon >> Distributions >> HLT_IsoTkMu18
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-10-06-1100+16118/16251/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/HLT_Muon_Distributions_HLT_IsoTkMu18.html

@slava77 from your experience with the comparison plots, is this an actual item to be worried about?

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2016

On 10/7/16 7:09 AM, Chris Jones wrote:

I looked through all the comparison plots and the only one with even a
minor difference was

25202.0 HLT >> Muon >> Distributions >> HLT_IsoTkMu18
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-10-06-1100+16118/16251/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/HLT_Muon_Distributions_HLT_IsoTkMu18.html

HLT_IsoTkMu was on our ignore list for probably 6 months.
Comparisons for it regularly show random differences.

@slava77 https://github.com/slava77 from your experience with the
comparison plots, is this an actual item to be worried about?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16118 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbjXbXeCQnESZSgT_lcc_ryQO1vryks5qxlKUgaJpZM4KQIv6.

@davidlange6 davidlange6 merged commit 5e5eaff into cms-sw:CMSSW_8_1_X Oct 8, 2016
@Dr15Jones Dr15Jones deleted the properUseOfErasForL1Reco branch November 9, 2016 07:59
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

4 participants