Modernize HGCalTriggerGeometryESProducer #27242
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27242/10436
|
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: L1Trigger/L1THGCal @cmsbuild, @rekovic, @benkrikler, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| const CaloGeometry* calo_geometry_ = nullptr; | ||
| const HGCalGeometry* hgc_ee_geometry_ = nullptr; | ||
| const HGCalGeometry* hgc_hsi_geometry_ = nullptr; | ||
| const HGCalGeometry* hgc_hsc_geometry_ = nullptr; |
There was a problem hiding this comment.
(repeating from the description) There is a slight change in behavior. Earlier, the existence of these products was checked by the isValid() or product() calls in above. This PR suggests to move those checks to the ESProducer. Is that ok?
There was a problem hiding this comment.
This looks fine to me (actually, probably slightly better than the previous approach)
| iRecord.getRecord<IdealGeometryRecord>().get("HGCalHEScintillatorSensitive", hsc_geometry); | ||
| geometry->initialize(ee_geometry, hsi_geometry, hsc_geometry); | ||
| geometry->initialize( | ||
| &iRecord.get(ee_geometry_token_), &iRecord.get(hsi_geometry_token_), &iRecord.get(hsc_geometry_token_)); |
There was a problem hiding this comment.
(repeating from the description) With the eventual (naive) ES-product prefetching, this implementation would always load/produce the CaloGeometry and the 3 HGCalGeometry objects. In practice (based on the comments above) the HGCalGeometry objects would be "extra" only in case of V7/V8 HGCal geometries. Are the V7/V8 geometries still relevant?
I presume the configurations for HGCalGeometryESProducer instances are always in the configuration (since that is the recommended pattern). Is it ok to run them for V7/V8 geometries? (just thinking if the price is only some CPU time and memory, or if something weird could happen as well)
There is a phase2_hgcalV9 modifier. Would it be acceptable to use that to control the behavior of this ESProducer?
There was a problem hiding this comment.
There was a problem hiding this comment.
this implementation would always load/produce the
CaloGeometryand the 3HGCalGeometryobjects
Does it mean that the method
bool isV9Geometry() const { return !calo_geometry_; }would always return false?
There was a problem hiding this comment.
this implementation would always load/produce the
CaloGeometryand the 3HGCalGeometryobjectsDoes it mean that the method
bool isV9Geometry() const { return !calo_geometry_; }would always return
false?
No, because in case of V7/V8 geometry only the calo_geometry_ member is set and thus the current behavior of HGCalTriggerGeometryBase is preserved. What I discussed in above was about how the production of the input ESProducts would change.
There was a problem hiding this comment.
In general, using the modifier is good practice to make it clear when the relevant access method should be employed.
I take this as a green light for using the modifier here. Do I interpret the modifier/era structure correctly that it is sufficient to use only phase2_hgcalV9, or should it be phase2_hgcalV9 | phase2_hgcalV10?
There was a problem hiding this comment.
The workflows with HGCal V10 also include the V9 modifier to reduce duplication, so it's fine to use just the V9 one.
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
The code-checks are being triggered in jenkins. |
|
Following #27242 (comment) I modified the ESProducer to use the |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27242/10479
|
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
Pull request #27242 was updated. @cmsbuild, @rekovic, @benkrikler, @kpedro88 can you please check and sign again. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+upgrade |
|
@rekovic this PR is waiting since one week. Do you have any comment? Otherwise I will integrate it in one of the next IBs |
|
+1 |
|
merge |
PR description:
In addition of using ESGetToken in HGCalTriggerGeometryESProducer (as part of #26748), this PR suggests to use raw pointers instead of
ESHandlefor the contained products in HGCalTriggerGeometryBase and derived classes.This change does alter the behavior, as now the existence of the input products is always checked in the ESProducer (earlier it was checked at the first time when the HGCalTriggerGeometryBase or -derived class accessed the input products).
There is one caveat though. Currently the ESProducer tries to read CaloGeometry, and if either CaloGeometry is not available or does not have the expected content, the ESProducer reads 3 HGCalGeometry products. With the solution in this PR, all 4 objects would get produced in the (eventual) prefetching, which is not necessarily what we want. On the other hand, (based on my understanding of the code) memory-wise this issue would only affect HGCal < V9 geometries, but I don't know how relevant those workflows still are. In addition, in principle theIn addition, the ESProducer is modified to use thephase2_hgcalV9modifier could be used to control the behavior of this ESProducer.phase2_hgcalV9modifier to control whether to read the CaloGeometry (for V7/V8 geometries) or 3 HGCalGeometry objects (V9 and beyond).PR validation:
Code compiles.