Phase2-hgx174 Bug fixes

[bsunanda]

(1) Adds a tolerance for comparison in double
(2) Provides the right dimension for corner edges

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25344/7364

[cmsbuild]

A new Pull Request was created by @bsunanda for master.

It involves the following packages:

Geometry/HGCalCommonData
Geometry/HGCalGeometry

@civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[bsunanda]

@cmsbuild Please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31848/console Started: 2018/11/26 15:53

[cmsbuild]

+1
Tested at: 3d71ab4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25344/31848/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 239 differences found in the comparisons
• DQMHistoTests: Total files compared: 33
• DQMHistoTests: Total histograms compared: 3131839
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3131633
• DQMHistoTests: Total skipped: 204
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
• Checked 137 log files, 14 edm output root files, 33 DQM output files

[cmsbuild]

+1
Tested at: 5202e16
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25344/31884/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 238 differences found in the comparisons
• DQMHistoTests: Total files compared: 33
• DQMHistoTests: Total histograms compared: 3131839
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3131634
• DQMHistoTests: Total skipped: 204
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
• Checked 137 log files, 14 edm output root files, 33 DQM output files

[bsunanda]

@cvuosalo @kpedro88 Can you sign this PR?

[kpedro88]

@bsunanda are the changes in layerClusters understood?

[bsunanda]

There were bugs in the earlier code (1) corners of cells are now better done and tested by Alexandros; (2) the neighbor code was not protected about crossing radius limits; (3) the radius as a function of Z is now correctly done (and tested). These are the 3 changes - I am not sure how any of the changes can affect layerClusters - if these functions are used in making layer clusters, the earlier results were wrong.

[kpedro88]

@rovere @clelange FYI - the changes are very small, so I don't think they will affect the cluster calibration, but please take a look

[cvuosalo]

@bsunanda: I think it is helpful to have the named constant that you added. Since this PR changed the implicit factor of 1.0 to an explicit 0.5, one could imagine that a future change might alter the factor again, maybe to 0.25. Having the named constant allows a developer to change the factor in one place rather than many places in the code. The name of the constant could reflect its purpose rather than its value, but there's no need to change its name right now.

[cvuosalo]

+1

[fabiocos]

@kpedro88 @rovere @clelange do we have a conclusion about the changes?

[rovere]

Hi all,
I looked at the changes and, given their size and the relative small statistics, I'd say they are "reasonable", i.e., I think they do not signal any problem. I have to admit, though, that I still do not understand how these changes could affect layerClusters, hence all quantities derived from them, but nothing underneath (basically RecHits).

[bsunanda]

@kpedro88 Could you please clear this PR? there is something else which has cropped up and i can investigate that once it is integrated

[kpedro88]

+upgrade

[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