proposal to move CaloVShape and derived objects to CondFormats or CalibFormats

[slava77]

Calorimeter pulse shapes are defined in a sim-specific subsystem SimCalorimetry/*SimAlgos (and SimFastTiming for BTL) and are used extensively outside, in the following subsystems
CalibCalorimetry, SimCalorimetry, SimFastTiming, RecoLocalCalo, RecoTBCalo, Validation.

The shapes are typically derived from DB objects or are hardcoded.
This appears to be a good case for a definition to be in a common/"upstream" subsystem package, and could be in the CondFormats (CalibFormats), e.g. CondFormats/CaloShapes or CondFormats/CaloShapesTransient.

The rationale for the relocation is to reduce interdependencies between subsystems.

Some relationship details are in the following:

SimCalorimetry/CaloSimAlgos/interface/CaloVShape.h:                   class CaloVShape {
    .. in SimCalorimetry/{CaloSimAlgos, CastorSim, EcalSimAlgos, HcalSimAlgos, HcalTestBeam, FastTimingCommon}, Validation/EcalDigis
    SimCalorimetry/CaloSimAlgos/interface/CaloCachedShapeIntegrator.h: class CaloCachedShapeIntegrator : public CaloVShape {
        .. in SimCalorimetry/{CaloSimAlgos, HcalSimAlgos}
    SimCalorimetry/CaloSimAlgos/interface/CaloShapeIntegrator.h:      class CaloShapeIntegrator : public CaloVShape {
        .. in SimCalorimetry/{CastorSim, CaloSimAlgos, HcalSimAlgos, HcalTestBeam}
    SimCalorimetry/CastorSim/src/CastorShape.h:                                 class CastorShape :  public CaloVShape {
        .. in SimCalorimetry/CastorSim
    SimCalorimetry/EcalSimAlgos/interface/ESShape.h:                        class ESShape : public CaloVShape {
        .. in SimCalorimetry/EcalSimAlgos, SimCalorimetry/EcalSimProducers, Validation/EcalDigis
    SimCalorimetry/EcalSimAlgos/interface/EcalShapeBase.h:              class EcalShapeBase : public CaloVShape {
        .. in CalibCalorimetry/Ecal* , RecoLocalCalo/EcalRecAlgos, SimCalorimetry/EcalSimAlgos
        SimCalorimetry/EcalSimAlgos/interface/EBShape.h:                         class EBShape : public EcalShapeBase
            .. in CalibCalorimetry/Ecal*, RecoLocalCalo/EcalRecProducers, RecoTBCalo/EcalTBRecProducers, SimCalorimetry/EcalSimProducers, Validation/EcalDigis
    SimCalorimetry/HcalSimAlgos/interface/HFShape.h:                        class HFShape : public CaloVShape {
        .. in SimCalorimetry/HcalSimAlgos
    SimCalorimetry/HcalSimAlgos/interface/HcalSiPMShape.h:             class HcalSiPMShape : public CaloVShape {
        .. in SimCalorimetry/HcalSimAlgos
    SimCalorimetry/HcalSimAlgos/interface/ZDCShape.h:                     class ZDCShape : public CaloVShape {
        .. in SimCalorimetry/HcalSimAlgos
    SimFastTiming/FastTimingCommon/interface/MTDShapeBase.h:   class MTDShapeBase : public CaloVShape {
        .. in SimFastTiming/FastTimingCommon
        SimFastTiming/FastTimingCommon/interface/BTLPulseShape.h: class BTLPulseShape : public MTDShapeBase {
            .. in SimFastTiming/FastTimingCommon

[cmsbuild]

A new Issue was created by @slava77 Slava Krutelyov.

@Dr15Jones, @silviodonato, @dpiparo, @smuzaffar, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[slava77]

assign alca,reconstruction,simulation

[cmsbuild]

New categories assigned: simulation,reconstruction,alca

@mdhildreth,@slava77,@christopheralanwest,@tlampen,@pohsun,@perrotta,@tocheng,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

[kpedro88]

As some additional input for this proposal, there's a separate HcalPulseShape class in CalibCalorimetry:
https://github.com/cms-sw/cmssw/blob/master/CalibCalorimetry/HcalAlgos/interface/HcalPulseShape.h

The code that manages the different HCAL pulse shapes is also quite a mess and would benefit from reorganization and database usage (this is a lingering cleanup item from the Phase 1 development):
https://github.com/cms-sw/cmssw/blob/master/CalibCalorimetry/HcalAlgos/interface/HcalPulseShapes.h

@abdoulline

[civanch]

I agree with the proposal. I would do this in 11_2.

[mariadalfonso]

is this new package update still targeting the 11_2 ?

[tvami]

Hi @slava77 has this ever been resolved?

[slava77]

Hi @slava77 has this ever been resolved?

not that I'm aware.

[tvami]

Thanks Slava. Would the Simulation meeting be a good place to revisit this proposal? @civanch what do you think?

[kpedro88]

Wherever it's discussed, the ECAL and HCAL DPGs should be informed in advance so that their software experts can provide feedback (since they're the primary users of these classes).

[tvami]

@cms-sw/hcal-dpg-l2 @cms-sw/ecal-dpg-l2 what's your preference?

[thomreis]

I do not really understand the argument about reducing the interdependencies. Now theses classes are located in SimCalorimetry and the other packages depend on that package. Afterwards they would be in another package (CondFormats or CalibFormats) and also all the other packages, including SimCalorimetry, would depend on the new one. There seems to be no improvement here.

[thomreis]

I might add that at the moment PRs are open to migrate some derived classes to esConsumes (e.g. #34902) so whatever is decided should maybe wait until the migration has finished.

[abdoulline]

(not saying for HCAL DPG, just my two cents)
I guess if somebody could move CaloVshape to a "neutral-ground" package to avoid a dependence of Reco and some other (aforementioned by Slava) packages on SimCalorimetry and would update all the relevant header links accordingly
https://cmssdt.cern.ch/lxr/search?_filestring=&_string=CaloVShape
then HCAL DPG shouldn't have objections about it.

[tvami]

@civanch if everybody agrees, would you please schedule a discussion about this at the next simulation meeting?

[tvami]

@civanch @cms-sw/simulation-l2 @cms-sw/ecal-dpg-l2 @cms-sw/hcal-dpg-l2 @kpedro88
should this be discussed at
https://indico.cern.ch/event/1071938/
?

[thomreis]

@civanch @cms-sw/simulation-l2 @cms-sw/ecal-dpg-l2 @cms-sw/hcal-dpg-l2 @kpedro88
should this be discussed at
https://indico.cern.ch/event/1071938/
?

That is fine for us (ECAL).

[tvami]

Follow-up: it was agreed that @bsunanda and @civanch will take care of this issue

[tvami]

Hi @bsunanda do you maybe have any news on this?

[tvami]

Hi @bsunanda will you have the time to do this soon? Should we aim to finish with this under 12_3_X?

[tvami]

Hi @bsunanda ,
when do you think it is realistic to deal with this?

[tvami]

Hi @civanch maybe this could be discussed in the Simulation meeting next week?
https://indico.cern.ch/event/1117488/

[tvami]

+alca

• AlCa agrees to the plan, but the implementation is up to the simulation group

[jpata]

any news on this?