Follow-up from "Simple calo correction"

The following discussion from !1764 (merged) should be addressed:

• @aszabels started a discussion: (+1 comment)

  What I see is that the multithread running has troubles with the line CaloFutureECorrection.cpp

  if ( m_caloElectron->set( tmp ) ) { incidence = m_caloElectron->caloState().momentum().Theta(); }

  when I try to print out the components of this line the I get a FATAL when running multithreaded. But even before the error strange values are assigned to 'incidence' for some events. I suspect caloState() to be the source of the problem but I don't understand how (it is of type LHCb::State). I need some help from experts here. Therefore I am not sure how big changes this requires and if this cures all problems with multithread. I am in favour to continue the work on this in a new MR to close the work that is already done.

The tool CaloFutureElectron has a state which is changed by set( tmp ). So by the time you want to get the state via caloState, another thread from a different event has overwritten it.

CaloFutureElectron has to be made thread-safe

added Calo label

The problem here is the global variables (CaloFutureElectron.cpp):

// call-dependent state -- set by 'set', and afterwards const -- so could be returned as data from 'set'...
  bool                  m_status        = false;
  const LHCb::Track*    m_track         = nullptr;
  const LHCb::CaloHypo* m_electron      = nullptr;
  const LHCb::CaloHypo* m_bremstrahlung = nullptr;
  LHCb::State           m_state;
  DeCalorimeter*        m_calo = nullptr;
};

which are shared by other public methods ( ICaloFutureElectron.h):

   virtual bool set( ProtoParticle const& proto, std::string const& det = DeCalorimeterLocation::Ecal,
                      CaloPlane::Plane plane = CaloPlane::ShowerMax, double delta = 0 ) = 0;

    virtual State           caloState() const                                                         = 0;
    virtual State           closestState() const                                                      = 0;
    virtual double          eOverP() const                                                            = 0;
    virtual const CaloHypo* electron() const                                                          = 0;
    virtual const CaloHypo* bremstrahlung() const                                                     = 0;
    virtual CaloMomentum    bremCaloFutureMomentum() const                                            = 0;
    virtual double          caloTrajectoryL( CaloPlane::Plane refPlane = CaloPlane::ShowerMax ) const = 0;
  };

'set' method is not const but in practice it uses only protoParticle as an input. My idea is to make 'set' a private method that returns an internal structure containing all the variables which are now global and that can be passed between the methods. Each of the above methods should take protoParticle (or a caloHypo and a track, as these are actually used) as an input parameter. What do you think?

mentioned in merge request Moore!305 (merged)

mentioned in issue Moore#118 (closed)

I do not know this code in details, so I may say something stupid, but why do you need set methods at all ? If all they do is set global variables, just drop the global variables and the calls to set methods and replace them by passing the ex global variables as references to the methods that were using the globals. Basically make all the arguments explicit, rather than hiding half of them in globals.

In order to do exactly what @sponce mentioned, I've already pushed the code to the point where there is a single set method that computes 'all' the observables, and could just return a struct -- there is just a little bit of work required so that the struct doesn't do too much work up front, as not all callers need all the information...

@graven could you point me to the branch you are referring to?

it's already in master ;-) Basically, I made a lot of changes in eg LHCb!2045 (merged) and !1642 (merged) which resulted in an interface which has one non-const method, and then a set of const accessors to the information determined by the one non-const one... before these merge requests, it was more complicated.

mentioned in merge request !1806 (merged)

mentioned in merge request LHCb!2239 (merged)