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

VID Feature Requests for 75X #9659

Merged
merged 78 commits into from Jul 10, 2015
Merged

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Jun 17, 2015

This pull request implements the following requested features in VID:

  • Validation tool to yield a checksum that validates the expected ID performance, in addition to the checksumming of configurations
  • A lightweight and straightforward tool for manipulating the bitmap output of the cuts (so that cuts can be masked on the fly in code)
  • Output of the values cut upon in addition to the cut results
  • Use of ExpressionEvaluator

The middle two points are implemented as "vid::CutFlowResult" which is a digest that implements a masking and value-retrieval interface.

This pull request now also implements "MINIAODfromMINIAOD" data reprocessing and VID-by-value, which only functions for objects with all necessary information embedded in some way (except for vertices, etc which are always in the event).

@ikrav @cmkuo @gpetruc

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_5_X.

VID Feature Requests for 75X

It involves the following packages:

CommonTools/Utils
DataFormats/PatCandidates
PhysicsTools/SelectorUtils
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification
RecoMuon/MuonIdentification

@cmsbuild, @cvuosalo, @vadler, @monttj, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @battibass, @makortel, @abbiendi, @jhgoh, @bellan, @gpetruc, @trocino, @bachtis, @rociovilar this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@slava77
Copy link
Contributor

slava77 commented Jun 18, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1
Tested at: 3d5a39b
When I ran the RelVals I found an error in the following worklfows:
25202.0 step2

runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25+MINIAODMCUP15/step2_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25+MINIAODMCUP15.log
----- Begin Fatal Exception 18-Jun-2015 20:25:12 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=MixingModule label='mix'
Exception Message:
RootEmbeddedFileSequence no input files specified for secondary input source.
----- End Fatal Exception -------------------------------------------------

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

@monttj
Copy link
Contributor

monttj commented Jul 10, 2015

+1

@davidlange6
Copy link
Contributor

50 MB is not something to just swallow.. (especially in 74x) @lgray can you confirm what this is?

On Jul 10, 2015, at 3:20 AM, Slava Krutelyov notifications@github.com wrote:

+1

for #9659 156b815

• code changes look OK (also, nothing additional showing up from the static analyzer)
• jenkins tests pass and comparisons with baseline show no difference (this covers the RECO step parts)
• locally tested in CMSSW_7_5_X_2015-07-07-2300 /test area sign566/, confirming jenkins comparison results and also check step5/miniAOD with 70 events in ttbar PU=35 (wflow 25202)
• photonMVAValueMapProducer and electronMVAValueMapProducer are added
• egmGsfElectronIDs time per event increases 0.48 ms/ev -> 3.43 ms/ev
• egmPhotonIDs time per event increases 0.24 ms/ev -> 1.32 ms/ev
• patPhotons_slimmedPhotons__PAT compressed size on disk is up by 8% and patElectrons_slimmedElectrons__PAT size is up by 15% (user data values added)
• peak RSS increases by 50 MB ... likely from a combination of the MVAs and the modifier instances

Reply to this email directly or view it on GitHub.

@lgray
Copy link
Contributor Author

lgray commented Jul 10, 2015

@davidlange6 3 ID MVAs being run for the new EGM IDs. Since this is on top of the usual memory usage of MiniAOD 50 MB is relatively large, but in absolute terms comparing to RSS limits I imagine it's not?

Notably this doesn't happen in 74X because these IDs are not run in MiniAOD production.

@davidlange6
Copy link
Contributor

On Jul 10, 2015, at 7:36 AM, Lindsey Gray notifications@github.com wrote:

@davidlange6 3 ID MVAs being run for the new EGM ID. Since this is ontop of the usual memory usage of MiniAOD 50 MB is relatively large, but in absolute terms comparing to RSS limits I imagine it's not?

is 3% of the total budget.. indeed its importance depends on where one is relative to the budget:) [which we are over partly due to many more memory intensive MVAs in reco] - but by 75x we can plan to be running multicore on the tier0.

Notably this doesn't happen in 74X because these IDs are not run.


Reply to this email directly or view it on GitHub.

@lgray
Copy link
Contributor Author

lgray commented Jul 10, 2015

OK, noted.
Since it's not mentioned, this is not a show stopper for 74X?

@davidlange6
Copy link
Contributor

On Jul 10, 2015, at 8:06 AM, Lindsey Gray notifications@github.com wrote:

OK, noted.
Since it's not mentioned, this is not a show stopper for 74X?

not to me.


Reply to this email directly or view it on GitHub.

@lgray
Copy link
Contributor Author

lgray commented Jul 10, 2015

@franzoni Could you please take a look here and +1 if you find things agreeable? Thanks!

davidlange6 added a commit that referenced this pull request Jul 10, 2015
@davidlange6 davidlange6 merged commit 420745b into cms-sw:CMSSW_7_5_X Jul 10, 2015
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