search

SegmentLinking / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1 stars 1 forks source link

Remove global variables #26

Closed ariostas closed 1 month ago

ariostas commented 1 month ago

This PR is the companion to https://github.com/SegmentLinking/TrackLooper/pull/401. It seems like it's working now, but I'll see if there's anything that needs to be cleaned up.

ariostas commented 1 month ago

@slava77 so should the return of produce be std::shared_ptr<SDL::LSTESData<SDL::Dev> const>?

I'm looking through some other examples and I'm not seeing the constness in ESProducers.

https://github.com/SegmentLinking/cmssw/blob/f9e0bcf7dc1af26f6d86843f43a023d303379317/RecoTracker/MkFit/plugins/MkFitGeometryESProducer.cc#L36

slava77 commented 1 month ago

@slava77 so should the return of produce be std::shared_ptr<SDL::LSTESData<SDL::Dev> const>?

I'm looking through some other examples and I'm not seeing the constness in ESProducers.

https://github.com/SegmentLinking/cmssw/blob/f9e0bcf7dc1af26f6d86843f43a023d303379317/RecoTracker/MkFit/plugins/MkFitGeometryESProducer.cc#L36

No, it's more about the internal content of LSTESData; access to the eventSetup will give access to a const object auto const& lstESData; but the problem is the pointers inside LSTESData currently allow the pointed data to be non-const. This is bad if the data is actually non-const.

Another point I brought up during the meeting is to use unique_ptr to leave the ownership to the event setup https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToWriteESProducer.

ariostas commented 1 month ago

I tried to separate the ESProducer into host and device parts, but I couldn't get it to work because it tries to do some copy from host to device and I couldn't bypass it. I left the structure commented out in case someone else might know what's wrong with it. For now, I switched to doing everything on the device producer even though it's a bit wasteful to duplicate some of the steps.

The current commit works, but I still have to address the review comments on the companion PR.