Bug fix in Halo code

[wddgit]

Fix a bug where the code is using the index
into the RefVector where the index into the
collection is expected. This will result in
Ref's to the wrong element being created.

[cmsbuild]

A new Pull Request was created by @wddgit (W. David Dagenhart) for CMSSW_7_4_X.

Bug fix in Halo code

It involves the following packages:

DQMOffline/JetMET
DataFormats/METReco
RecoMET/METProducers

@nclopezo, @cvuosalo, @slava77, @cmsbuild, @deguio, @danduggan can you please review it and eventually sign? Thanks.
@rappoccio, @ahinzmann, @mmarionncern, @nhanvtran, @schoef, @mariadalfonso, @TaiSakuma, @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.

[wddgit]

The same bug is repeated in 4 different places.

I am not familiar with this code. It is possible
this code is not used for anything, but if it is
used then it is likely these bugs are creating
incorrect results. I did not test this beyond
making sure the fix compiles and it would be a
good idea for someone familiar with this to take
a look at it.

The Ref constructor used by this code will deleted
from the Core code in a pull request which will be
submitted within the next few days. That is why I
noticed this. That pull request will depend on this
one getting merged first.

[slava77]

why was this a bug?
Ref(RefVector<C, T, F> const& refvector, key_type itemKey) exists
is the point that it's becoming a bug with updates of the interface?

[Dr15Jones]

Say the code was

std::vector<Foo*> allFoos;
std::vector<Foo*> someFoos( aFunctionThatMakesChoices(allFoos) );

for(unsigned int i = 0; i< someFoos.size(); i++) {
   std::cout << allFoos[i];
}

Would you consider using the wrong size to be a bug? That is exactly what the code was doing.

[wddgit]

For example, the RefVector might have 5 entries and the TrackCollection might have 1000 tracks. The 3rd element of the RefVector might reference the 500th track. The existing constructor is passing an index into the RefVector into a function that expects and index into the TrackCollection, In this example, passing 3 instead of 500.

[slava77]

Oh, indeed.
Now it's obvious

[ghost]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.

[cmsbuild]

+1
Tested at: 617d784
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7433/2207/summary.html

[slava77]

looking at jenkins tests, the halo variables didn't change.
I wonder if anything shows up with more events ...
AFAIK, the code is not completely unused.
Maybe Tai or Robert could comment on use cases

[deguio]

+1

[slava77]

+1

for #7433 617d784
tested in CMSSW_7_4_0_pre6 /test area sign505, cherry-picked/

With 100 events (the default in the short matrix with MC recycling) I actually got a small difference.
So, the code outputs actually have some dependence on the bug.

TTbar Run1 MC no PU (25.0):

Apparently, there are no DQM plots for this product.

@TaiSakuma May I ask you to review the use cases of this product in the MET meetings and see if it's still useful.

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request will be automatically merged.