Fix addUserInt and addUserFloat with multiple values on the same key #12750

gpetruc · 2015-12-10T15:54:04Z

#11574 didn't implement correctly the cases of multiple values on the same key:

addUserFloat was ignoring the overwrite parameter
the overwrite parameter was not present in the addUserInt, only in addUserFloat
when not overwriting, the code was inserting the new value but not inserting a duplicate label, that was causing the label vector and value vector to go off sync.

Should not affect MiniAOD production nor MiniAOD analysis, since I believe in those workflows we never write multiple values with the same key.

If the investigation of https://hypernews.cern.ch/HyperNews/CMS/get/physTools/3435.html reveals further problems, I will add them in this PR.

Please have a look, if satisfactory I can make also 76X backport and, if needed by HI, 75X backport.

@lgray

cmsbuild · 2015-12-10T15:54:25Z

A new Pull Request was created by @gpetruc (Giovanni Petrucciani) for CMSSW_8_0_X.

It involves the following packages:

DataFormats/PatCandidates

@cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please review it and eventually sign? Thanks.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

+1|approve[d]|sign[ed]: L1/L2's to approve it
-1|reject[ed]: L1/L2's to reject it
assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
hold: L1/all L2's/release manager to mark it as on hold
unhold: L1/user who put this PR on hold
merge: L1/release managers to merge this request
[@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
[@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

slava77 · 2015-12-10T16:06:03Z

@cmsbuild please test

Giovanni, I think you can make a 76X PR already

cmsbuild · 2015-12-10T16:06:18Z

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

gpetruc · 2015-12-10T17:44:15Z

Realized that addUserData also needs a fix, which unfortunately needs more
lines of code since edm::OwnVector doesn't have a way to do assignment
with proper semantics (vec[i] = a will call operator= on the pointee of
data[i], i.e. try to overwrite the value, and so it can't be used to
replace an old pointer with a new pointer)

…item

cmsbuild · 2015-12-10T18:56:50Z

Pull request #12750 was updated. @smuzaffar, @Dr15Jones, @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

cmsbuild · 2015-12-10T18:58:05Z

-1
Tested at: 17c121f
When I ran the RelVals I found an error in the following worklfows:
4.22 step2

runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log

8.0 step3

runTheMatrix-results/8.0_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS/step3_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS.log

134.911 step3

runTheMatrix-results/134.911_RunSinglePh2015D+RunSinglePh2015D+HLTDR2_25ns+RECODR2_25nsreHLT+HARVESTDR2_25nsreHLT/step3_RunSinglePh2015D+RunSinglePh2015D+HLTDR2_25ns+RECODR2_25nsreHLT+HARVESTDR2_25nsreHLT.log

140.53 step2

runTheMatrix-results/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log

25.0 step3

runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT/step3_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT.log

1000.0 step2

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step2_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log

1001.0 step2

runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4.log

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

gpetruc · 2015-12-10T19:02:14Z

Updated so that now the add methods with overwrite=false will add an entry.
It required extending a bit the OwnVector interface, but in a trivial way.

RelVal error seem to be unrelated, they seem to fail at configuration creation not finding the alca config for the Charmonium PD or something similar.

cvuosalo · 2015-12-11T01:25:22Z

@cmsbuild please test
Try again

cmsbuild · 2015-12-11T01:25:41Z

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

cmsbuild · 2015-12-11T03:12:00Z

-1
Tested at: b25910a
I found an error when building:

In file included from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/DataFormats/PatCandidates/interface/Lepton.h:22:0,
                 from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/DataFormats/PatCandidates/interface/Muon.h:27,
                 from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/PhysicsTools/PatAlgos/test/private/PATUserDataTestModule.cc:35:
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/DataFormats/PatCandidates/interface/PATObject.h: In instantiation of 'void pat::PATObject::addUserData(const string&, const T&, bool, bool) [with T = ROOT::Math::LorentzVectorROOT::Math::PxPyPzE4D; ObjectType = reco::Muon; std::string = std::basic_string]':
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/PhysicsTools/PatAlgos/test/private/PATUserDataTestModule.cc:129:60:   required from here
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/DataFormats/PatCandidates/interface/PATObject.h:310:91: error: no matching function for call to 'pat::PATObjectreco::Muon::addUserDataObject_(const string&, std::auto_ptrpat::UserData, bool&)'
         addUserDataObject_( label, pat::UserData::make(data, transientOnly), overwrite );
                                                                                           ^
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/DataFormats/PatCandidates/interface/PATObject.h:310:91: note: candidate is:
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2015-12-10-1100/src/DataFormats/PatCandidates/interface/PATObject.h:771:8: note: void pat::PATObject::addUserDataObject_(const string&, pat::UserData*, bool) [with ObjectType = reco::Muon; std::string = std::basic_string]
   void PATObject::addUserDataObject_( const std::string & label, pat::UserData \* data, bool overwrite )

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

cmsbuild · 2015-12-15T21:04:35Z

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

…ends

cmsbuild · 2015-12-16T10:33:55Z

Pull request #12750 was updated. @smuzaffar, @Dr15Jones, @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

cvuosalo · 2015-12-16T17:42:00Z

@cmsbuild please test

cmsbuild · 2015-12-16T17:42:20Z

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

cmsbuild · 2015-12-17T01:35:22Z

-1

Tested at: 4b799cf
I found errors in the following unit tests:

---> test testRecoMETMETProducers had ERRORS

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

cmsbuild · 2015-12-17T02:20:34Z

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

Dr15Jones · 2015-12-17T13:05:45Z

+1
The OwnVector changes are fine.
The test failure is unrelated and comes from the IB.

cvuosalo · 2015-12-17T17:08:08Z

+1

For #12750 4b799cf

Bug fix to allow addUserInt and addUserFloat to handle multiple values with the same key. There should be no change in monitored quantities.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2015-12-16-1100 show no significant differences, as expected. The Jenkins unit test failure is unrelated to this PR.

Fix addUserInt and addUserFloat with multiple values on the same key

Fix addUserInt and addUserFloat with multiple keys

17c121f

cmsbuild added this to the Next CMSSW_8_0_X milestone Dec 10, 2015

cmsbuild added reconstruction-pending analysis-pending pending-signatures tests-pending orp-pending comparison-pending labels Dec 10, 2015

cmsbuild added tests-started and removed tests-pending labels Dec 10, 2015

gpetruc added 4 commits December 10, 2015 19:18

DataFormats/Common OwnVector: support for insertion and resetting an …

e010b4c

…item

DataFormats/Common OwnVector: add unint tests

f78dbd0

PATObject: fix addUserData with multiple values

0963ec0

Overwrite set to false will add a new entry, also for userCand

b25910a

cmsbuild added core-pending tests-pending and removed tests-started labels Dec 10, 2015

cmsbuild added tests-started and removed tests-pending labels Dec 11, 2015

cmsbuild removed the tests-started label Dec 11, 2015

cmsbuild added tests-rejected and removed tests-started labels Dec 15, 2015

cmsbuild added comparison-available and removed comparison-pending labels Dec 15, 2015

OwnVector iterator: make methods using base iterator public, make fri…

4b799cf

…ends

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-rejected labels Dec 16, 2015

cmsbuild added tests-started and removed tests-pending labels Dec 16, 2015

cmsbuild added tests-rejected and removed tests-started labels Dec 17, 2015

cmsbuild added comparison-available and removed comparison-pending labels Dec 17, 2015

cmsbuild added core-approved and removed core-pending labels Dec 17, 2015

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Dec 17, 2015

davidlange6 added a commit that referenced this pull request Dec 21, 2015

Merge pull request #12750 from gpetruc/userIntsFloats_multiKey_fix_76X

bac9ce7

Fix addUserInt and addUserFloat with multiple values on the same key

davidlange6 merged commit bac9ce7 into cms-sw:CMSSW_8_0_X Dec 21, 2015

gpetruc deleted the userIntsFloats_multiKey_fix_76X branch June 1, 2016 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix addUserInt and addUserFloat with multiple values on the same key #12750

Fix addUserInt and addUserFloat with multiple values on the same key #12750

gpetruc commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

slava77 commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

gpetruc commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

gpetruc commented Dec 10, 2015

cvuosalo commented Dec 11, 2015

cmsbuild commented Dec 11, 2015

cmsbuild commented Dec 11, 2015

cmsbuild commented Dec 15, 2015

cmsbuild commented Dec 16, 2015

cvuosalo commented Dec 16, 2015

cmsbuild commented Dec 16, 2015

cmsbuild commented Dec 17, 2015

cmsbuild commented Dec 17, 2015

Dr15Jones commented Dec 17, 2015

cvuosalo commented Dec 17, 2015

Fix addUserInt and addUserFloat with multiple values on the same key #12750

Fix addUserInt and addUserFloat with multiple values on the same key #12750

Conversation

gpetruc commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

slava77 commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

gpetruc commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

cmsbuild commented Dec 10, 2015

gpetruc commented Dec 10, 2015

cvuosalo commented Dec 11, 2015

cmsbuild commented Dec 11, 2015

cmsbuild commented Dec 11, 2015

cmsbuild commented Dec 15, 2015

cmsbuild commented Dec 16, 2015

cvuosalo commented Dec 16, 2015

cmsbuild commented Dec 16, 2015

cmsbuild commented Dec 17, 2015

cmsbuild commented Dec 17, 2015

Dr15Jones commented Dec 17, 2015

cvuosalo commented Dec 17, 2015