cms-patatrack / cmssw

CMSSW fork of the Patatrack project
https://patatrack.web.cern.ch/patatrack/index.html
Apache License 2.0
2 stars 5 forks source link

Update GPU HCAL conditions framework #574

Closed fwyzard closed 3 years ago

fwyzard commented 3 years ago

Address the comments from review of cms-sw#32039.

Remove duplicate GPU-related HCAL conditions records, and simplify the package dependencies moving the remaining ones to CondFormats/DataRecord.

Improve the handling of the GPU conditions payloads:

fwyzard commented 3 years ago

If these changes look reasonable, I plan to extend them from HcalConvertedPedestalWidthsGPU to the other ESProducts.

fwyzard commented 3 years ago

@mariadalfonso @vkhristenko if you are fine with the changes, I'll work on the other ESProducts next.

vkhristenko commented 3 years ago

np from my side

On Thu, 19 Nov 2020 at 19:35, Andrea Bocci notifications@github.com wrote:

@mariadalfonso https://github.com/mariadalfonso @vkhristenko https://github.com/vkhristenko if you are fine with the changes, I'll work on the other ESProducts next.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/cms-patatrack/cmssw/pull/574#issuecomment-730560879, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSFUCKNU4YAAZ3NT5BTPZDSQVQQVANCNFSM4T3L3I3Q .

mariadalfonso commented 3 years ago

looks ok to me. I have not tested the changes but I assume are transparent

-  setup.get<HcalConvertedEffectivePedestalWidthsRcd>().get(effectivePedestalWidthsHandle);
+  setup.get<HcalConvertedPedestalWidthsRcd>().get(effectivePedestalWidthsHandle);
mariadalfonso commented 3 years ago

by the way similarly should be done for the conditions in ECAL

fwyzard commented 3 years ago

Validated on top of CMSSW_11_2_0_pre9

image

fwyzard commented 3 years ago

OK, I think I've addressed all the comments on edm::propage_const_array ?

fwyzard commented 3 years ago

According to StackOverflow the defaulted constructors are automatically constexpr, if possible.

I can add it explicitly if you prefer, it compiles fine.

Dr15Jones commented 3 years ago

According to StackOverflow the defaulted constructors are automatically constexpr, if possible.

I did a quick check on godbolt and StackOverflow appears correct :).

I can add it explicitly if you prefer, it compiles fine.

I guess I don't mind either way.

fwyzard commented 3 years ago

I'll add it - for consistency with https://en.cppreference.com/w/cpp/experimental/propagate_const/propagate_const :-)

.A