Run2-hcx192 Changes to DDHCalAngular by removing explicit reference to CLHEP #26271

bsunanda · 2019-03-27T18:12:59Z

PR description:

Remove explicit reference to CLHEP from DDHCalAngular and also making changes to debug statements

PR validation:

Run the code with debug On which prints out the positioning with translation and rotation in this algorithm for all calls in the new and the older versions and make a diff which results no difference:

diff -bw /build/sunanda/CMSSW_10_6_X_2019-03-26-2300/relval/dump2.log dump.log

1c1
< Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 27-Mar-2019 18:59:23.481 CET

Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 27-Mar-2019 19:01:00.661 CET

if this PR is a backport please specify the original PR:

Need to go to master and is the first step before translating this for dd4hep

cmsbuild · 2019-03-27T18:13:29Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-03-27T18:19:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26271/8953

This PR adds an extra 12KB to repository

bsunanda · 2019-03-27T18:19:36Z

@cmsbuild Please test

cmsbuild · 2019-03-27T18:21:04Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33809/console Started: 2019/03/27 19:22

cmsbuild · 2019-03-27T18:21:06Z

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

It involves the following packages:

Geometry/HcalAlgo

@civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild 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

cmsbuild · 2019-03-27T20:36:10Z

+1
Tested at: b0eb2bf
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26271/33809/summary.html

cmsbuild · 2019-03-27T20:36:15Z

Comparison job queued.

cmsbuild · 2019-03-27T21:58:37Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2869 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3114829
DQMHistoTests: Total failures: 3320
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3111312
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 133 log files, 14 edm output root files, 32 DQM output files

cvuosalo · 2019-03-28T16:34:21Z

I still don't understand the source of the differences. You say the positioning is not changed by this PR. Then why are there differences?
And the number of differences in this PR are about half of those in #26272. Why?

davidlange6 · 2019-03-28T16:56:24Z

here is some python code that might be useful...

def oldC(phi):
if phi>0: iphi=int(phi+0.1)
else: iphi=int(phi-0.1)

if iphi>=360: iphi-=360
return float(iphi)

def newC(phi):
iphi=round(phi)
if iphi>= 360:
iphi-=360
return float(iphi)

for i in range(0,4000):
deg=i/10.
print deg,oldC(deg),newC(deg)

davidlange6 · 2019-03-28T16:58:38Z

Geometry/HcalAlgo/plugins/DDHCalAngular.cc

      } 
    }

-    double xpos = shiftX*cos(phi) - shiftY*sin(phi);
-    double ypos = shiftX*sin(phi) + shiftY*cos(phi);
+    double xpos = shiftX*cos(phix) - shiftY*sin(phix);


otherwise, this is only equivalent to the old implementation for integer values of convertRadToDeg(startAngle)

I have made some tests with new vs old implementation and then test new==old. You see this test fail in many cases. Theta is 90 degree. pi and twopi match. All iphi's are the same but phideg or phi (phix) do not in many cases.

Theta 1.5708:1.5708:1
Pi 3.14159:3.14159:1 TwoPi 6.28319:6.28319:1
phideg 270:270:1 iphi 270:270 phi 4.71239:4.71239:1
phideg 300:300:1 iphi 300:300 phi 5.23599:5.23599:1
phideg 330:330:0 iphi 330:330 phi 5.75959:5.75959:0
phideg 0:0:1 iphi 0:0 phi 0:0:1
phideg 30:30:0 iphi 30:30 phi 0.523599:0.523599:1
phideg 60:60:0 iphi 60:60 phi 1.0472:1.0472:1
phideg 90:90:1 iphi 90:90 phi 1.5708:1.5708:1
phideg 120:120:0 iphi 120:120 phi 2.0944:2.0944:1
phideg 150:150:0 iphi 150:150 phi 2.61799:2.61799:0
phideg 180:180:0 iphi 180:180 phi 3.14159:3.14159:0
phideg 210:210:0 iphi 210:210 phi 3.66519:3.66519:0
phideg 240:240:0 iphi 240:240 phi 4.18879:4.18879:1

So my conclusion is that these are all rounding effects and once there is random # sequence goes crazy.

@bsunanda: Could you please explain the print-out above with phideg, iphi, etc.? I don't see any differences.

The first # is the new value, the second is the old value and the third is (old == new). I did not put (old==new) for int32 like iphi. But in many cases old==new fails and shows 0 which is due to some rounding issues. This is the culprit for change is random # sequence. This always causes difference for small samples but statistically they balance out

Frankly I am now not worried about this difference

It would be helpful to see (old - new) when they differ to understand the scale of the differences.

The difference is at the level of e-14. Here they are. For each, there are the new value, old value, difference and test of new==old results
Theta 1.5708:1.5708:0:1
Pi 3.14159:3.14159:0:1 TwoPi 6.28319:6.28319:0:1
phideg 270:270:0:1 iphi 270:270:0:1 phi 4.71239:4.71239:0:1
phideg 300:300:0:1 iphi 300:300:0:1 phi 5.23599:5.23599:0:1
phideg 330:330:5.68434e-14:0 iphi 330:330:0:1 phi 5.75959:5.75959:8.88178e-16:0
phideg 0:0:0:1 iphi 0:0:0:1 phi 0:0:0:1
phideg 30:30:-3.55271e-15:0 iphi 30:30:0:1 phi 0.523599:0.523599:0:1
phideg 60:60:-7.10543e-15:0 iphi 60:60:0:1 phi 1.0472:1.0472:0:1
phideg 90:90:0:1 iphi 90:90:0:1 phi 1.5708:1.5708:0:1
phideg 120:120:-1.42109e-14:0 iphi 120:120:0:1 phi 2.0944:2.0944:0:1
phideg 150:150:-2.84217e-14:0 iphi 150:150:0:1 phi 2.61799:2.61799:-4.44089e-16:0
phideg 180:180:-2.84217e-14:0 iphi 180:180:0:1 phi 3.14159:3.14159:-4.44089e-16:0
phideg 210:210:-2.84217e-14:0 iphi 210:210:0:1 phi 3.66519:3.66519:-8.88178e-16:0
phideg 240:240:-2.84217e-14:0 iphi 240:240:0:1 phi 4.18879:4.18879:0:1

Geometry/HcalAlgo/plugins/DDHCalAngular.cc

cmsbuild · 2019-04-09T12:04:36Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34074/console Started: 2019/04/09 14:05

ianna · 2019-04-09T12:16:57Z

@cvuosalo - you can check if it has any impact on calo geometry by running the following validation: https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideDDDversusDataBaseValidation#Calo_Geometry

cmsbuild · 2019-04-09T14:10:35Z

+1
Tested at: ca03edf
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26271/34074/summary.html

cmsbuild · 2019-04-09T14:10:43Z

Comparison job queued.

cmsbuild · 2019-04-09T16:11:15Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 5980 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3140495
DQMHistoTests: Total failures: 7909
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3132389
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 133 log files, 14 edm output root files, 32 DQM output files

cvuosalo · 2019-04-09T16:49:40Z

+1

I am still perplexed about how the very tiny change in this PR related to rounding could cause thousands of differences. But if no one else sees a problem, I won't hamper progress.

For the record, here is Sunanda's explanation:
The non-matching in small samples is a well known phenomenon in simulation world. Random # sequence gets modified and the differences get reduced only with large statistics.

cmsbuild · 2019-04-09T16:50:16Z

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)

bsunanda · 2019-04-10T01:35:43Z

@fabiocos Could you please merge this PR. Like to use one of the methods in DDTypes to subsequent PR's which are in queue

bsunanda · 2019-04-12T13:50:11Z

@fabiocos Could you please merge this PR?

fabiocos · 2019-04-12T20:08:54Z

+1

differences affecting upgrade workflows, it would be better to cross check the explanation of @bsunanda with a higher statistics test

fabiocos · 2019-04-12T20:17:32Z

@bsunanda the differences are seen only in upgrade workflows because they are producing geometry from DD on the fly, and not using snapshots from DB. Your explanation sounds plausible, but do you have a test independent on MC simulation dumping the geometry positions and showing that the differences are marginal?

bsunanda · 2019-04-13T00:51:34Z

@fabiocos I printed the geometry positions and found out that they had identical values in print outs.

fabiocos · 2019-04-14T16:52:21Z

@bsunanda ok, thanks, that is the check that I was interested in

Changes to DDHCalAngular by removing explicit reference to CLHEP

b0eb2bf

cmsbuild added this to the CMSSW_10_6_X milestone Mar 27, 2019

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures tests-pending labels Mar 27, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 27, 2019

cmsbuild added tests-started and removed tests-pending labels Mar 27, 2019

cmsbuild added tests-approved and removed tests-started labels Mar 27, 2019

cmsbuild added comparison-available and removed comparison-pending labels Mar 27, 2019

davidlange6 reviewed Mar 28, 2019

View reviewed changes

cvuosalo reviewed Mar 28, 2019

View reviewed changes

Geometry/HcalAlgo/plugins/DDHCalAngular.cc Outdated Show resolved Hide resolved

cvuosalo mentioned this pull request Apr 4, 2019

Run2-hcx194 Changes to some of the algorithms used for TB application by removing explicit reference to CLHEP #26351

Merged

Convert to degree only to find the name of rot matrix

3c37ae7

cmsbuild removed the tests-approved label Apr 9, 2019

cmsbuild added comparison-pending tests-pending tests-started and removed tests-pending labels Apr 9, 2019

cmsbuild added tests-approved and removed tests-started labels Apr 9, 2019

cmsbuild added comparison-available and removed comparison-pending labels Apr 9, 2019

cmsbuild added fully-signed geometry-approved and removed geometry-pending pending-signatures labels Apr 9, 2019

cvuosalo mentioned this pull request Apr 9, 2019

Run2-hcx193 Changes to DDHCalBarrelAlgo by removing explicit reference to CLHEP #26272

Merged

cmsbuild added orp-approved and removed orp-pending labels Apr 12, 2019

cmsbuild merged commit 2ce801f into cms-sw:master Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run2-hcx192 Changes to DDHCalAngular by removing explicit reference to CLHEP #26271

Run2-hcx192 Changes to DDHCalAngular by removing explicit reference to CLHEP #26271

bsunanda commented Mar 27, 2019 •

edited

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

bsunanda commented Mar 27, 2019

cmsbuild commented Mar 27, 2019 •

edited

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

cvuosalo commented Mar 28, 2019

davidlange6 commented Mar 28, 2019 •

edited

davidlange6 Mar 28, 2019

bsunanda Mar 28, 2019

cvuosalo Mar 28, 2019

bsunanda Mar 28, 2019

bsunanda Mar 28, 2019

cvuosalo Mar 28, 2019

bsunanda Mar 28, 2019

cmsbuild commented Apr 9, 2019 •

edited

ianna commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

cvuosalo commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

bsunanda commented Apr 10, 2019

bsunanda commented Apr 12, 2019

fabiocos commented Apr 12, 2019

fabiocos commented Apr 12, 2019

bsunanda commented Apr 13, 2019

fabiocos commented Apr 14, 2019

Run2-hcx192 Changes to DDHCalAngular by removing explicit reference to CLHEP #26271

Run2-hcx192 Changes to DDHCalAngular by removing explicit reference to CLHEP #26271

Conversation

bsunanda commented Mar 27, 2019 • edited

PR description:

PR validation:

1c1 < Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 27-Mar-2019 18:59:23.481 CET

if this PR is a backport please specify the original PR:

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

bsunanda commented Mar 27, 2019

cmsbuild commented Mar 27, 2019 • edited

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

cmsbuild commented Mar 27, 2019

cvuosalo commented Mar 28, 2019

davidlange6 commented Mar 28, 2019 • edited

davidlange6 Mar 28, 2019

Choose a reason for hiding this comment

bsunanda Mar 28, 2019

Choose a reason for hiding this comment

cvuosalo Mar 28, 2019

Choose a reason for hiding this comment

bsunanda Mar 28, 2019

Choose a reason for hiding this comment

bsunanda Mar 28, 2019

Choose a reason for hiding this comment

cvuosalo Mar 28, 2019

Choose a reason for hiding this comment

bsunanda Mar 28, 2019

Choose a reason for hiding this comment

cmsbuild commented Apr 9, 2019 • edited

ianna commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

cvuosalo commented Apr 9, 2019

cmsbuild commented Apr 9, 2019

bsunanda commented Apr 10, 2019

bsunanda commented Apr 12, 2019

fabiocos commented Apr 12, 2019

fabiocos commented Apr 12, 2019

bsunanda commented Apr 13, 2019

fabiocos commented Apr 14, 2019

bsunanda commented Mar 27, 2019 •

edited

1c1
< Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 27-Mar-2019 18:59:23.481 CET

cmsbuild commented Mar 27, 2019 •

edited

davidlange6 commented Mar 28, 2019 •

edited

cmsbuild commented Apr 9, 2019 •

edited