HGCAL Electronics mapping (v2) #44729
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44729/39940
|
|
A new Pull Request was created by @pfs for master. It involves the following packages:
@Dr15Jones, @civanch, @makortel, @mdhildreth, @srimanob, @cmsbuild, @bsunanda, @mandrenguyen, @jfernan2, @saumyaphor4252, @consuegs, @perrotta, @francescobrivio, @subirsarkar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test with cms-data/Geometry-HGCalMapping#1 |
|
type hgcal |
|
-1 Failed Tests: UnitTests Unit TestsI found 1 errors in the following unit tests: ---> test testHGCalMapFileParser had ERRORS Comparison SummarySummary:
|
cmsbuild
added
tests-pending
code-checks-pending
and removed
code-checks-approved
labels
|
please test with cms-data/Geometry-HGCalMapping#1 |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44729/39947
|
|
Pull request #44729 was updated. @mandrenguyen, @makortel, @subirsarkar, @srimanob, @civanch, @consuegs, @bsunanda, @mdhildreth, @francescobrivio, @jfernan2, @perrotta, @Dr15Jones, @saumyaphor4252 can you please check and sign again. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-43aac2/38832/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Comparison SummarySummary:
|
| @@ -0,0 +1 @@ | |||
| #include "CondFormats/HGCalObjects/src/headers.h" | |||
There was a problem hiding this comment.
Why is CondFormats/HGCalObjects/src/headers.h needed at all?
Just #include the necessary here directly and remove the file headers.h (as @makortel commented in the original PR)
There was a problem hiding this comment.
Ok, there was some discussion about it in the original PR, which unfortunately gets lost once passing to a new PR.
In any case, the comment of including here the necessary header files without passing through the inclusion of the intermediate header.h remains
There was a problem hiding this comment.
thanks - indeed we followed strictly as recommended in https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideCondObjectsTutorial . Other variations tried resulted in compilation errors
There was a problem hiding this comment.
What I found out in #43751 (comment) is that scram b has a specific "CondFormat serialization" phase that requires the headers.h to exist. I'd suggest to document that requirement better :)
| @@ -39,27 +42,31 @@ class HGCalElectronicsId { | |||
| */ | |||
| HGCalElectronicsId() : value_(0) {} | |||
| explicit HGCalElectronicsId( | |||
There was a problem hiding this comment.
The explicit keyword was removed in the previous PR.
It is probably good to keep it, I just wanted to point out the differences between this and the original PR
There was a problem hiding this comment.
Indeed, in the previous PR Ihave made such mistake. explicit has been introduced in PR #44484
| @@ -0,0 +1,16 @@ | |||
| <bin file="testHGCalMapFileParser.cc"> | |||
| <flags TEST_RUNNER_ARGS=" Geometry/HGCalMapping/data/ModuleMaps/modulelocator_CEminus_V15p5.txt"/> | |||
There was a problem hiding this comment.
This line is new wrt #43751
There was a problem hiding this comment.
yes, this test unit has been added and forces cms-data/Geometry-HGCalMapping#1 to be used explicitly
|
No need to duplicate - conflicts solved in #43751. |
This PR supersedes #43751. It is rebased on top of
CMSSW_14_1_0_pre2and it's made of a single commit which squashes all the ones in #43751. Below the description and the validation of #43751 are copied and updatedPR description:
Adding the infrastructure needed to
The tool has been discussed in
It relies on the mappings in cms-data/Geometry-HGCalMapping#1
The requests to leave EVENTSETUP_RECORD_REG and COND_SERIALIZABLE commented are not implemented as they generate compilation errors and break the testers. The HGCAL DPG is however aware that the move to CondDB is to occur in a future time-scale (end of this year/begin on next year).
PR validation:
This PR is not expected to impact any workflow thus we expect 100% success in the tests. The PR is however accompanied by a series of unit tests and analyzers (under the test) sub-directories. They have been used to validate the content produced by the new pieces of code being added to the repository.
@kerstinlovisa @jalimena