Evaluate replacing ECAL CalibratedClusterPtr with CalibratedPFCluster

[fwyzard]

The PFECALSuperClusterAlgo class uses internally a CalibratedPFCluster class which is a wrapper around an edm::Ptr<reco::PFCluster>:

  class CalibratedPFCluster {
  public:
    CalibratedPFCluster(const edm::Ptr<reco::PFCluster>& p) : cluptr(p) {}

    double energy() const { return cluptr->correctedEnergy(); }
    double energy_nocalib() const { return cluptr->energy(); }
    double eta() const { return cluptr->positionREP().eta(); }
    double phi() const { return cluptr->positionREP().phi(); }

    edm::Ptr<reco::PFCluster> the_ptr() const { return cluptr; }

  private:
    edm::Ptr<reco::PFCluster> cluptr;
  };

As well as a shared_ptr<...> and a vector<shared_ptr<...>> types:

  typedef std::shared_ptr<CalibratedPFCluster> CalibratedClusterPtr;
  typedef std::vector<CalibratedClusterPtr> CalibratedClusterPtrVector;

The use of what are essentially a shared_ptr<Ptr<T>> and vector<shared_ptr<Ptr<T>>> seems inefficient, given that edm::Ptr<T> is a lightweight object which should be used by value.

In addition, the use of std::vector<std::shared_ptr<CalibratedPFCluster>> could be replaced by the much more efficient edm::PtrVector<T>.

The use of these non optimal data structures is exacerbated by #37115, which introduces and uses

     typedef std::vector<std::pair<CalibratedClusterPtr, CalibratedClusterPtrVector>> EcalGraphOutput;

vs the more compact

     typedef std::vector<std::pair<edm::Ptr<reco::PFCluster>, edm::PtrVector<reco::PFCluster>>> EcalGraphOutput;

I would suggest to study the impact of replacing CalibratedClusterPtr with just CalibratedPFCluster, and of reworking the interface to support using edm::PtrVector<reco::PFCluster>.

[fwyzard]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

@jpata,@slava77,@clacaputo you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Issue was created by @fwyzard Andrea Bocci.

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

cms-bot commands are listed here

[fwyzard]

type performance-improvements

[fwyzard]

assign egamma-pog

[cmsbuild]

New categories assigned: egamma-pog

@lfinco,@swagata87 you have been requested to review this Pull request/Issue and eventually sign? Thanks

[fwyzard]

type egamma

[swagata87]

Thanks a lot Andrea, for the suggestions.
Together with Davide, we will try to address this soon.

[swagata87]

Thanks Davide for addressing this issue in #40460

[valsdav]

I wanted to discuss more in details the strategy to follow for CalibratedClusterPtrVector. As a first step I have already moved from vector<shared_ptr<edm::Ptr<reco::PFCluster>>> to vector<<edm::Ptr<reco::PFCluster>>, but I'm not sure how to go for edm::PtrVector while keeping the CalibratedCluster interface for the corrected energy.

We can create a CalibratedClusterVector class encapsulating the edm::PtrVector and returning instances of CalibratedPFCluster. Is this the way you foreseen @fwyzard ?

[fwyzard]

@valsdav my naive approach would have been to just get rid of the CalibratedPFCluster class, and use the native energy() and correctedEnergy() methods.

If, as I understand, e/gamma prefers to keep the CalibratedPFCluster interface, encapsulating an edm::PtrVector<reco::PFCluster>> in a new class CalibratedClusterVector, like you suggest, seems like a good idea.

[valsdav]

Hi @fwyzard at the end I am trying to just get rid of the CalibratedClusterVector and just use edm::PtrVector<reco::PFCluster> and rework the egamma code to use correctedEnergy() instead of energy().

The problem I'm facing is that the code is using the std::vector interface for the Mustache algorithm and the edm::PtrVector is missing those methods (erasing, copying a vector from a range). https://github.com/cms-sw/cmssw/blob/master/RecoEcal/EgammaClusterAlgos/src/PFECALSuperClusterAlgo.cc#L405

I think I can just solve it by manually creating and filling a new PtrVector. Is there some utility to manipulate the PtrVectors in cmssw that I don't know? Thanks for your help.

[fwyzard]

@valsdav what methods would you need from PtrVector ?
I think it would be reasonable to extent the PtrVector interface to provide similar functionality to std::vector.

[fwyzard]

Also, I just found that DataFormats/Common/interface/fillPtrVector.h provides various fillPtrVector() functions - but I've never used them.

[makortel]

Also, I just found that DataFormats/Common/interface/fillPtrVector.h provides various fillPtrVector() functions - but I've never used them.

That header file has a comment

cmssw/DataFormats/Common/interface/fillPtrVector.h

Line 13 in 79e79bd

This is an internal detail of edm::Ptr interaction with edm::Wrapper and should not be used by others

so I'd recommend to avoid using it.

[valsdav]

The PFECALSuperClusterAlgo code is basically using:

• erase
• std::stable_partition
• constructor(begin_it,end_it)