PPS: follow-up geometry fixes

[jan-kaspar]

This issue is to list the suggested fixes in PPS geometry code that

• started to be discussed in Port CTPPS to DD4hep #31240,
• we decided to defer them to a dedicated follow-up PR.

[cmsbuild]

A new Issue was created by @jan-kaspar .

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

cms-bot commands are listed here

[ghugo83]

As mentioned by @markotel, there are issues in PPS legacy code which should be addressed (usual memory management issues, like pointed-to objects not deleted, etc). They have been there for several years in legacy code.

They do not lead to any regression nor crash from any test after move of PPS to DD4hep (quite independent from this).
Though, it would be good anyway to have a clean PPS package (also a clean CMSSW actually...).

Here are the points to address, as (mostly) mentioned by @makortel (links to lines already in master):

• Pointed-to objects not deleted in https://github.com/cms-sw/cmssw/blob/master/Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc#L121 , should use smart pointers anyway.
• Would be clearer to add return type to https://github.com/cms-sw/cmssw/blob/master/Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc#L121
• Exception handling at https://github.com/cms-sw/cmssw/blame/master/Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc#L345
• Potential double-delete at https://github.com/cms-sw/cmssw/blame/master/Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc#L371 and https://github.com/cms-sw/cmssw/blame/master/Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc#L380
• Will need to use ESGetToken soon when ready at https://github.com/cms-sw/cmssw/blob/master/CondTools/Geometry/plugins/PPSGeometryBuilder.cc#L68
  ( https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#In_ED_module , https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#Getting_data_from_EventSetup_wit ) (although we're almost done providing a simpler way to call esConsumes())

[jan-kaspar]

Many thanks @ghugo83 !

[jan-kaspar]

There are also comments by @davidlange6 :

• Port CTPPS to DD4hep #31240 (comment)
• Port CTPPS to DD4hep #31240 (comment)
  Some of them are already addressed in add missing includes to CondFormats/GeometryObjects #31343

[silviodonato]

assign geometry

[cmsbuild]

New categories assigned: geometry

@Dr15Jones,@cvuosalo,@mdhildreth,@makortel,@ianna,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

[ghugo83]

Finished cleaning all these points.
Except the following: I do not understand which object would be double-deleted, in legacy code in:

std::unique_ptr<CTPPSGeometry> PPSGeometryESProducer::produceRealTG(const VeryForwardRealGeometryRecord& iRecord) {
  auto const& gD = iRecord.get(dgdRealToken_);

  return std::make_unique<CTPPSGeometry>(&gD, verbosity_);
}

[ghugo83]

@makortel you said:

This will lead to double-delete. If an ESProduct is intended to be forwarded as such, it should be done as
return std::shared<CTPPSGeometry const>(&gD, edm::do_nothing_deleter());

[ghugo83]

But which object would be double-deleted?
&gd is a raw, observing pointer to a DetGeomDesc.
We have a unique_ptr to a CTPPSGeometry object.

Sorry for asking explanation ;p But maybe you thought that &gD was the forward of a CTPPSGeometry object?

[makortel]

@ghugo83 Sorry, you're right, I got confused. The two objects (gD and the return value of produceRealTG()) are indeed separate. Sorry for the noise.

[jan-kaspar]

So if I understand correctly, @ghugo83's set of fixes is complete (many thanks!). When would it be a good time to issue this PR (having all the other developments going on in parallel)?

[ghugo83]

@jan-kaspar
Am there: https://github.com/cms-sw/cmssw/compare/master...ghugo83:parallel_oldDD_DD4hep?expand=1

Basically this afternoon, I have:

• integrated the fixes mentioned in this page.
• moved out the custom constructor from PDetGeomDesc.
• restored old DD and made all the changes to have old DD and DD4hep in parallel (that was obviously the very heavy part).

Evth compiles and runs.
I still need to re-do all the tests on DetIds / coords on all volumes, and fix accordingly.
But ok there is just one hour left haha

[jan-kaspar]

@ghugo83 Huge thanks! Do I understand correctly that you plan to propose one big PR will all the fixes and improvements?

[ghugo83]

@jan-kaspar The small fixes, mentioned in the context of port to DD4hep, are included in #31383
This new PR does not touch old DD scenario, and instead add DD4hep capability in parallel.

[jan-kaspar]

Many thanks @ghugo83 - does this mean that this issue can be closed? Or is some more work needed for the old DD scenarios?

[ghugo83]

Yes I would say can be closed, first 4 points are addressed.
Only cheeky point is the 5th point, which was not compulsory. I must say I have used ESGetToken in the ESProducer, but in the Analyzer, I have kept the legacy way of getting CompactView:

 ESHandle<SetupData> pSetup;
 iSetup.get<SetupRecord>().get(pSetup);

[ghugo83]

Ok I have just fully addressed the 5th point as well. Will push the commit shortly.
As soon as the commit is pushed, I guess you can close this issue, as absolutely all points here have been looked at and addressed (except point 4 which was not relevant).

[jan-kaspar]

Thanks @ghugo83 !