HGCalWaferValidation: refinements and D86 geometry validation fix #36502

imranyusuff · 2021-12-15T03:46:59Z

PR description:

Changelog Tue, 14 Dmr 2021 by Imran Yusuff:

Further tidying the code
Now uses the first line of the flat file to determine flat file type (single number means new format)
Separate out shape and rotation matching operation into functions
Fixed validation for D86 geometry (especially in rotation for type-3 layers)

PR validation:

Now correctly validates wafer with layerType = 3:

... snip ...
ROTATION ERROR: (33,12,3)  ( 0 != 5 (0) )  name=hgcalwafer:HGCalHEWafer0Coarse2
ROTATION ERROR: (33,12,9)  ( 0 != 5 (0) )  name=hgcalwafer:HGCalHEWafer0Coarse2
ROTATION ERROR: (33,-3,9)  ( 0 != 5 (0) )  name=hgcalwafer:HGCalHEWafer0Coarse2
ROTATION ERROR: (33,-9,3)  ( 0 != 5 (0) )  name=hgcalwafer:HGCalHEWafer0Coarse2
ROTATION ERROR: (33,-9,-12)  ( 0 != 5 (0) )  name=hgcalwafer:HGCalHEWafer0Coarse2
ROTATION ERROR: (33,-3,-12)  ( 0 != 5 (0) )  name=hgcalwafer:HGCalHEWafer0Coarse2

*** ERROR COUNTS ***
Missing         :  0
Thickness error :  0
Pos-x error     :  0
Pos-y error     :  0
Shape error     :  0
Rotation error  :  6
Unaccounted     :  0

Total wafers processed from geotxtfile = 13545

There are at least one wafer error.
%MSG-i DDCompactViewImpl:  AfterModEndJob 15-Dec-2021 04:28:58 CET EndJob
DDD transient representation has been destructed.
%MSG

However, there still remain 6 rotation errors, all on layer number 33.

Testing with D77 and D83 geometries produced unchanged results.

No other bugs found during testing.

(- Imran -)

cmsbuild · 2021-12-15T03:55:53Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36502/27377

This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36502/27377/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36502/27377/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-12-15T04:16:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36502/27378

This PR adds an extra 20KB to repository

cmsbuild · 2021-12-15T04:17:13Z

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

It involves the following packages:

Validation/HGCalValidation (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@vandreev11, @sethzenz, @bsunanda, @rovere, @lgray, @cseez, @apsallid, @pfs, @lecriste, @hatakeyamak, @ebrondol this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

jfernan2 · 2021-12-15T07:24:31Z

@imranyusuff can you please add yourself along with your github username in the comments filed to the HGCAL Validation developers e-group to keep track of the people working on it?
https://twiki.cern.ch/twiki/bin/viewauth/CMS/DQMContacts#HGCAL
Thanks

imranyusuff · 2021-12-15T09:54:03Z

@imranyusuff can you please add yourself along with your github username in the comments filed to the HGCAL Validation developers e-group to keep track of the people working on it?
https://twiki.cern.ch/twiki/bin/viewauth/CMS/DQMContacts#HGCAL
Thanks

Hi, I have just done that and is now waiting approval.

jfernan2 · 2021-12-15T12:55:50Z

Validation/HGCalValidation/plugins/HGCalWaferValidation.cc

+  if (geoThickClass == 0 && fileThickness == 120)
+    return true;
+  if (geoThickClass == 1 && fileThickness == 200)
+    return true;


aren't these ifs else if ?

Given the return statements, else if would act exactly the same

jfernan2 · 2021-12-15T12:57:00Z

Validation/HGCalValidation/plugins/HGCalWaferValidation.cc

+                                                    : waferShapeStr == "2" ? HGCalTypes::WaferPartialType::WaferChopTwoM
+                                                    : waferShapeStr == "3" ? HGCalTypes::WaferPartialType::WaferSemi2
+                                                    : waferShapeStr == "4" ? HGCalTypes::WaferPartialType::WaferSemi2
+                                                    : waferShapeStr == "5" ? HGCalTypes::WaferPartialType::WaferFive2


I am not sure if this kind of structures are oreferred over else ifs, I let experts to comment
@perrotta @qliphy

Same comment applies here, both for the if and for the map.
Also, labelling evolution as new and old will tell us nothing the moment we have more than 2 versions. It would be better to either express is clearly with version numbers or use other labels.

jfernan2 · 2021-12-15T12:57:34Z

Validation/HGCalValidation/plugins/HGCalWaferValidation.cc

+                            : shapeStr == "am" ? HGCalTypes::WaferPartialType::WaferHalf2
+                            : shapeStr == "b"  ? HGCalTypes::WaferPartialType::WaferFive
+                            : shapeStr == "bm" ? HGCalTypes::WaferPartialType::WaferFive2
+                            : shapeStr == "c"  ? HGCalTypes::WaferPartialType::WaferThree


I am not sure if this kind of structures are oreferred over else ifs, I let experts to comment
@perrotta @qliphy

To be honest, it is the first time I see these long chains. Tabbed as such it is still quite readable, in my opinion (it wouldn't be with a different indentation, though). I've personally nothing against it,

I prefer this way because:

Makes things more readable, in table-like form

Allows assignment to a const, which makes things more robust

Surely the changed code is more readable

I kind of agree with the comment from @jfernan2
From the performance point of view, there's no gain.
From the readability, the if-else is definitely more readable.
On a related note, instead of having this huge cascade of cases, would it make sense to have a map that, given the shapeStr will return the correct WaferPartialType? This would make the code even more readable.

You mean define a constant std::map within this module?

Then we would need 3 maps for that. One for old format mapping and DD, one for newer format low-density mapping and another one for newer format high-density mapping.

Yes, a std::map would do the job perfectly.
Keep in mind that the handling of the old format is just temporary and, as soon as we converge on something close to final, we can simply drop the rest.

Thanks!

Thanks! Will implement this now.

jfernan2 · 2021-12-15T13:28:20Z

please test

rovere · 2021-12-15T15:05:21Z

assign hgcal-dpg

cmsbuild · 2021-12-15T15:05:43Z

New categories assigned: hgcal-dpg

@felicepantaleo,@rovere,@pfs,@cseez you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild · 2021-12-15T17:10:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab9735/21286/summary.html
COMMIT: 75cf070
CMSSW: CMSSW_12_3_X_2021-12-14-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36502/21286/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250719
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3250691
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 41 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

imranyusuff · 2021-12-16T18:15:49Z

@rovere now I have implemented std::maps for wafer shape code mapping. But for now I have not made a check yet for invalid codes which might cause std::out_of_range exception. Do you think it is important to make such a check, in case an invalid DD entry or flat file is given?

cmsbuild · 2021-12-16T18:18:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36502/27426

This PR adds an extra 20KB to repository

cmsbuild · 2021-12-16T18:18:29Z

Pull request #36502 was updated. @pfs, @cseez, @felicepantaleo, @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @rovere, @pmandrik, @pbo0, @rvenditti can you please check and sign again.

jfernan2 · 2021-12-17T07:20:25Z

please test

cmsbuild · 2021-12-17T10:59:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab9735/21346/summary.html
COMMIT: 1735785
CMSSW: CMSSW_12_3_X_2021-12-16-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36502/21346/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2853 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250719
DQMHistoTests: Total failures: 2522
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3248175
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: found differences in 1 / 41 workflows

jfernan2 · 2021-12-17T12:27:58Z

+1

rovere · 2021-12-17T13:39:46Z

Validation/HGCalValidation/plugins/HGCalWaferValidation.cc

-        waferInfo.shapeCode = HGCalTypes::WaferPartialType::WaferChopTwo;
-      else if (shapeStr == "gm")
-        waferInfo.shapeCode = HGCalTypes::WaferPartialType::WaferChopTwoM;
+      waferInfo.shapeCode = waferShapeMapDD.at(shapeStr);


at has bounds-checking and, therefore, is slower. Since this is not reconstruction code, we can skip that for now and eventually use the direct [] access operator for later.

Yes, I did try to use [] first but failed to compile because it cannot accept const std::map.

rovere · 2021-12-17T13:43:40Z

@jfernan2 do you understand the 2522 differences?

jfernan2 · 2021-12-17T13:46:12Z

They are ONLY in wf 11634.7 which is misbehaving in all the PRs active around at present:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2021-12-16-2300+ab9735/47547/dqm-histo-comparison-summary.html

rovere · 2021-12-17T13:47:18Z

Thanks a lot for the info!

rovere · 2021-12-17T13:47:25Z

+1

cmsbuild · 2021-12-17T13:47:41Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-12-17T17:07:05Z

+1

HGCalWaferValidation: refinements and D86 geometry validation fix

b0369ef

cmsbuild added this to the CMSSW_12_3_X milestone Dec 15, 2021

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Dec 15, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Dec 15, 2021

apply code-format patch

75cf070

cmsbuild added code-checks-pending and removed code-checks-rejected labels Dec 15, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 15, 2021

jfernan2 reviewed Dec 15, 2021

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Dec 15, 2021

cmsbuild added the hgcal-dpg-pending label Dec 15, 2021

cmsbuild removed the tests-started label Dec 15, 2021

cmsbuild added dqm-pending tests-pending labels Dec 16, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 16, 2021

cmsbuild added tests-started and removed tests-pending labels Dec 17, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 17, 2021

cmsbuild added dqm-approved and removed dqm-pending labels Dec 17, 2021

rovere reviewed Dec 17, 2021

View reviewed changes

cmsbuild added fully-signed hgcal-dpg-approved and removed pending-signatures hgcal-dpg-pending labels Dec 17, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 17, 2021

cmsbuild merged commit 2b7e9b6 into cms-sw:master Dec 17, 2021

HGCalWaferValidation: refinements and D86 geometry validation fix #36502

HGCalWaferValidation: refinements and D86 geometry validation fix #36502

Conversation

imranyusuff commented Dec 15, 2021 • edited Loading

PR description:

PR validation:

cmsbuild commented Dec 15, 2021

cmsbuild commented Dec 15, 2021

cmsbuild commented Dec 15, 2021

jfernan2 commented Dec 15, 2021

imranyusuff commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfernan2 commented Dec 15, 2021

rovere commented Dec 15, 2021

cmsbuild commented Dec 15, 2021

cmsbuild commented Dec 15, 2021

Comparison Summary

imranyusuff commented Dec 16, 2021

cmsbuild commented Dec 16, 2021

cmsbuild commented Dec 16, 2021

jfernan2 commented Dec 17, 2021

cmsbuild commented Dec 17, 2021

Comparison Summary

jfernan2 commented Dec 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rovere commented Dec 17, 2021

jfernan2 commented Dec 17, 2021

rovere commented Dec 17, 2021

rovere commented Dec 17, 2021

cmsbuild commented Dec 17, 2021

perrotta commented Dec 17, 2021

imranyusuff commented Dec 15, 2021 •

edited

Loading