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

Reduced muon validation for PhaseII Upgrade high PU scenarios (90X) #16669

Merged
merged 10 commits into from Nov 29, 2016

Conversation

calabria
Copy link
Contributor

@calabria calabria changed the title Reduced muon validation for PhaseII Upgrade high PU scenarios Reduced muon validation for PhaseII Upgrade high PU scenarios (90X) Nov 17, 2016
@cmsbuild cmsbuild added this to the Next CMSSW_9_0_X milestone Nov 17, 2016
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @calabria (Cesare) for CMSSW_9_0_X.

It involves the following packages:

Validation/RecoMuon

@cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@battibass, @abbiendi, @jhgoh, @calderona, @HuguesBrun, @trocino, @rociovilar this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@kpedro88
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

Pull request #16669 was updated. @civanch, @mdhildreth, @dmitrijus, @cmsbuild, @vanbesien, @davidlange6 can you please check and sign again.

@civanch
Copy link
Contributor

civanch commented Nov 17, 2016

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 17, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 17, 2016

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • 20034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D1_GenSimHLBeamSpotFull+DigiFull_2023D1+RecoFullGlobal_2023D1+HARVESTFullGlobal_2023D1
  • 21234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D4_GenSimHLBeamSpotFull+DigiFull_2023D4+RecoFullGlobal_2023D4+HARVESTFullGlobal_2023D4
  • 23234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D5_GenSimHLBeamSpotFull+DigiFull_2023D5+RecoFullGlobal_2023D5+HARVESTFullGlobal_2023D5

@dmitrijus
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Nov 25, 2016

+1

for #16669 5029b88

  • code changes are as intended
  • jenkins tests pass and comparisons show differences in 2023 wflows in muon isolation and calorimeter quantities (expected)

See more notes on technical performance in #16670 (comment)

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

@slava77
Copy link
Contributor

slava77 commented Nov 28, 2016

@davidlange6
are there any issues with this PR or is it just to early to expect it to be merged?

@@ -58,6 +58,13 @@ def customiseFor14833(process):
producer.includeME0 = cms.bool(False)
return process

def customiseFor16670(process):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @calabria @slava77 -does not a fillDescriptions implementation avoid this function completely? In any case, when will this get propagated to the menu?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we plan to migrate the HLT menu to CMSSW 9.0.x once pre1 is out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason a fillDescription does not work here? (with the nice feature of being desirable for the long term)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a typical problem of no easy fix.
There is not fillDescriptions for this tool.

It's an ESProducer, it currently gets a tool to be configured from the plugin factory during ::produce call, the needed parameter is a sub-tool of the tool to be configured.
So, some significant development and some refactoring might be necessary.

@Dr15Jones do we have an example of what to do in this case?

@davidlange6
Copy link
Contributor

davidlange6 commented Nov 29, 2016 via email

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Nov 29, 2016

At HLT we have currently exactly 5 parameters for any DetIdAssociatorESProducer instance
in the menu, even

process.muonDetIdAssociator = cms.ESProducer( "DetIdAssociatorESProducer",
  ComponentName = cms.string( "MuonDetIdAssociator" ),
  etaBinSize = cms.double( 0.125 ),
  nEta = cms.int32( 48 ),
  nPhi = cms.int32( 48 ),
  includeBadChambers = cms.bool( False )
)

So any missing parameters must either already be inserted by fillDescriptions, or via existsAs treated in the code, or conditionally on the value of ComponentName.

@Dr15Jones
Copy link
Contributor

@slava77 to my knowledge, we've never come up with a satisfactory mechanism of dealing with configuration parameters from internal plugins of modules.

@fwyzard
Copy link
Contributor

fwyzard commented Nov 29, 2016

Indeed.

Would it make sense to add a static fillDescriptions-like method to the classes that make use of the plugin system ?

@slava77
Copy link
Contributor

slava77 commented Nov 29, 2016

@davidlange6
would existsAs solution be acceptable to you?
(it was suggested by @Martin-Grunewald )
This is simple enough that I can see it implemented quickly.

@fwyzard
Copy link
Contributor

fwyzard commented Nov 29, 2016

I'd rather we finally have a general way of handling plugins, say in a month or so, and live with a python customisation in the meantime.

@davidlange6
Copy link
Contributor

davidlange6 commented Nov 29, 2016 via email

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 916211f into cms-sw:CMSSW_9_0_X Nov 29, 2016
@Dr15Jones
Copy link
Contributor

@fwyzard I don't have any plans for a 'general way' of handling plugins. The difficulty is the validator object created in the fillDescriptions method of the module would have to know how to load the plugin itself (talking to a different interface connected to the plugin's fillDescription rather than the regular plugin interface) and then know what parameters from the module must be passed to the plugin.

The extremely wide diversity of how plugins are configured (on top of the pervasiveness of plugins) makes a general solution extremely difficult.

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

10 participants