`fillDescriptions` for all PFlow producers and better handling of `HcalPFCuts`

[missirol]

#43025 updated several PFlow producers to read thresholds for HCal RecHits from the EventSetup (via the object HcalPFCuts), as opposed to reading them as a list of values from the PSet of each producer.

In my superficial reading of #43025, I pointed out that the implementation could be improved (see https://github.com/cms-sw/cmssw/pull/43025#discussion_r1367295288). There are a few aspects, which are potentially related.

• When one configures a module to read (HBHE) thresholds from the DB post-#43025, can one also empty the list of thresholds which are currently in the PSet, since those numbers will not be used ? If not, the HLT menus will have to carry around those numbers (dummy or not) for no reason.

  • For example, in [1] could one set thresholdsByDetector = cms.VPSet() ? It looks like HcalPFCuts will override what is in gatheringThreshold, but if one wants to apply cuts via gatheringThresholdPt, then gatheringThreshold will have to be a vector of the same length (it cannot be emptied).
  • Another example is [2]: here it looks like HcalPFCuts (if enabled) will take precedence over the values in logWeightDenominator, and it seems possible to use logWeightDenominatorByDetector = cms.VPSet() (empty) in future HLT menus, but I do not know if this will actually work.

• #43025 (comment): the use of a pointer to percolate HcalPFCuts in #43025 to different methods seems fragile to me (maybe it's just me). I was naively expecting a single utility class (U) which can be used to return these thresholds in client code, reading them either from the PSet or from the EventSetup (via HcalPFCuts). Any client needing these thresholds (e.g. LocalMaximumSeedFinder) would have a class member of type U which would return the right threshold for a given "detId".

• In https://github.com/cms-sw/cmssw/pull/43025#issuecomment-1790464916, it was also noted that some of the plugins touched by #43025 do not have a fillDescriptions method (e.g. PFClusterProducer). Maybe as a result of this, I see PF code using calls to (edm::ParameterSet).exists and (edm::ParameterSet).existsAs (e.g. here), which should be dropped in favour of fillDescriptions wherever possible. Overall, the lack of fillDescriptions makes it harder to understand what is actually needed in all these (nested) PSets.

[1]

    initialClusteringStep = cms.PSet( 
      thresholdsByDetector = cms.VPSet( 
        cms.PSet(  gatheringThreshold = cms.vdouble( 0.4, 0.3, 0.3, 0.3 ),
          gatheringThresholdPt = cms.vdouble( 0.0, 0.0, 0.0, 0.0 ),
          depths = cms.vint32( 1, 2, 3, 4 ),
          detector = cms.string( "HCAL_BARREL1" )
        ),
        cms.PSet(  gatheringThreshold = cms.vdouble( 0.1, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2 ),
          gatheringThresholdPt = cms.vdouble( 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0 ),
          depths = cms.vint32( 1, 2, 3, 4, 5, 6, 7 ),
          detector = cms.string( "HCAL_ENDCAP" )
        )
      ),

[2]

    pfClusterBuilder = cms.PSet( 
      minFracTot = cms.double( 1.0E-20 ),
      stoppingTolerance = cms.double( 1.0E-8 ),
      positionCalc = cms.PSet( 
        minAllowedNormalization = cms.double( 1.0E-9 ),
        posCalcNCrystals = cms.int32( 5 ),
        algoName = cms.string( "Basic2DGenericPFlowPositionCalc" ),
        logWeightDenominatorByDetector = cms.VPSet( 
          cms.PSet(  logWeightDenominator = cms.vdouble( 0.4, 0.3, 0.3, 0.3 ),
            depths = cms.vint32( 1, 2, 3, 4 ),
            detector = cms.string( "HCAL_BARREL1" )
          ),
          cms.PSet(  logWeightDenominator = cms.vdouble( 0.1, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2 ),
            depths = cms.vint32( 1, 2, 3, 4, 5, 6, 7 ),
            detector = cms.string( "HCAL_ENDCAP" )
          )
        ),

[cmsbuild]

A new Issue was created by @missirol Marino Missiroli.

@antoniovilela, @smuzaffar, @makortel, @Dr15Jones, @sextonkennedy, @rappoccio can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

assign reconstruction, pf, hlt

@cms-sw/hcal-dpg-l2

[cmsbuild]

New categories assigned: reconstruction,pf,hlt

@Martin-Grunewald,@mmusich,@jfernan2,@mandrenguyen,@bellan,@kdlong,@swagata87 you have been requested to review this Pull request/Issue and eventually sign? Thanks

[wang-hui]

assign reconstruction, pf, hlt

@cms-sw/hcal-dpg-l2

also adding @igv4321 and @abdoulline to the thread

[swagata87]

type pf

[mmusich]

@cms-sw/pf-l2 can you clarify if any attempt has been done yet in the direction of solving this issue?

[cmsbuild]

cms-bot internal usage

[swagata87]

as far as I'm aware, nothing much is done about the possible code improvements / cleanups etc suggested here. Let me tag PF reco L3 @stahlleiton who can look into the code improvements etc.

[swagata87]

thanks @stahlleiton for https://github.com/cms-sw/cmssw/pull/45212

[stahlleiton]

thanks @stahlleiton for #45212

This is only the first PR for this issue. I expect more PRs to come (for instance fixing exist and existAs, and other suggestions mentioned in this issue). Thought it was better to do it in multiple PRs than having one large PR

[Dr15Jones]

My opinion is multiple small PRs are better than one large.

[mmusich]

Thought it was better to do it in multiple PRs than having one large PR

clearly there are several issues lumped in the same github ticket here, so it is fine to address each of the main bullets in https://github.com/cms-sw/cmssw/issues/43169#issue-1974025170 separately. The "fixing exist and existAs" OTOH is intimately connected with supplying the fillDescriptions, thus it is my opinion it would better fit the same PR.

[stahlleiton]

Thought it was better to do it in multiple PRs than having one large PR

clearly there are several issues lumped in the same github ticket here, so it is fine to address each of the main bullets in https://github.com/cms-sw/cmssw/issues/43169#issue-1974025170 separately. The "fixing exist and existAs" OTOH is intimately connected with supplying the fillDescriptions, thus it is my opinion it would better fit the same PR.

Ok, I will address the exist issues in the same PR as the fill descriptions