Changed set to vector in PFDisplacedVertexSeed

[Dr15Jones]

A vector should reduce memory and most likely speed up the use of PFDisplacedVertexSeed.

Added unit tests for the code I touched.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23546/5135

[Dr15Jones]

I happen to have noticed this while trying to understand the IB failures using gcc 8. The use of the set bothered me so I changed it to a vector to improve its memory and CPU efficiency.

[Dr15Jones]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28601/console Started: 2018/06/10 00:18

[cmsbuild]

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

It involves the following packages:

DataFormats/ParticleFlowReco
RecoParticleFlow/PFTracking

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @makortel, @rovere, @lgray, @seemasharmafnal, @cbernet, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[cmsbuild]

+1
Tested at: e5e6489
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23546/28601/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 45 differences found in the comparisons
• DQMHistoTests: Total files compared: 31
• DQMHistoTests: Total histograms compared: 2902751
• DQMHistoTests: Total failures: 46
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2902515
• DQMHistoTests: Total skipped: 190
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
• Checked 128 log files, 14 edm output root files, 31 DQM output files

[Dr15Jones]

As best as I can see from looking at the RECO comparisons, the extremely slight differences (usually just a shifting of an entry from one histogram bin to a neighboring bin) could be explained by slight precision shifts in the values.

[perrotta]

Hi @Dr15Jones : a set is ordered according to its element keys, while a vector is not. As a consequence, the order in which elements_ are accessed differs now, and with some debug I could verify that in a few cases also the displacedVertex's get a different order with this PR wrt the baseline.

Still, it is not obvious to me how do those different orders can be responsible of the tiny changes that you notice in a few reco comparisons. Do you have an idea about how this can happen?

You talk about some different precision, which could indeed be responsible of the tiny changes observed: where such a different precision is implemented in this PR?

[slava77]

On 6/15/18 2:12 AM, perrotta wrote: Hi @Dr15Jones <https://github.com/Dr15Jones> : a set is ordered according to its element keys, while a vector is not. As a consequence, the order in which |elements_| are accessed differs now, and with some debug I could verify that in a few cases also the |displacedVertex|'s get a different order with this PR wrt the baseline. Still, it is not obvious to me how do those different orders can be responsible of the tiny changes that you notice in a few reco comparisons. Do you have an idea about how this can happen? You talk about some different precision, which could indeed be responsible of the tiny changes observed: where such a different precision is implemented in this PR?

due to roundoffs, results of operations made on the same values in a different order can be different. Is a change of order actually meaningful? There should be no code that assumes ordering

[Dr15Jones]

@slava77 @perrotta is there still debate going on about the minor change of values?

[perrotta]

@Dr15Jones : well, I was waiting that you pointed out to me where there is such a variable saved in a different precision in your code (if this was what you meant by "slight precision shifts in the values")

On the other hand, we agrees that the change of order of few of the displacedVertex's (due to the change of order of the elements_ of the Set vs Vector) could be deemed responsible of the tiny shifts seen in the outputs. And, by looking at the code I don't think that the order that was originally decided for the std::set <...> elements_ had any other meaning rather than help in avoiding double counting (as it was mentioned in the comment in https://github.com/cms-sw/cmssw/pull/23546/files#diff-6903ba4b1023e45802608d40feee031aL36)

Therefore, if you have no objections, I could consider me ready to sign this off for reco

[Dr15Jones]

@perrotta When I was saying 'slight precision shifts in the values' I just meant that if the code was doing math operations (say additions) based on the order of the items in the set, then changing the order can make slight changes to the values at the smallest precision scale. I had not meant to imply some change to saved values. Sorry for the confusion.

Yes, please go ahead and sign off.

[perrotta]

+1

• Technical update
• Due to the different order in which the seeds are stored some tiny difference can be propagated to the reco outputs, as it is visible in some of the jenkins tests outputs

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

[fabiocos]

+1