CTPPS DetGeomDesc review

[ianna]

Review DetGeomDesc and address other issues mentioned in PR #25359

[cmsbuild]

A new Issue was created by @ianna Ianna Osborne.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[ianna]

assign @jan-kaspar

[ianna]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

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

[ianna]

assign geometry

[cmsbuild]

New categories assigned: geometry

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

[jan-kaspar]

@fabferro FYI: this is the discussion I mentioned yesterday at the meeting.

[slava77]

@jan-kaspar @fabferro
was there some progress on this issue?
Please clarify.
Thank you.

[jan-kaspar]

Hi @slava77 , not yet - we are still busy with preparations for the UL re-reco. This issue is already included on our TODO list:
CTPPS#40
for the calmer period afterwards. I hope this is OK.

[smuzaffar]

any update on this?

[cvuosalo]

The work is still in progress, according to item 12 in this spreadsheet:
https://docs.google.com/spreadsheets/d/1Yv_2uDdqvFGLTjspm60fnUMQfLr3geuNF_xXfpd_uWI/edit?usp=sharing

[jan-kaspar]

I confirm, Wagner's working on this task. Here's his last status report:
https://indico.cern.ch/event/922849/contributions/3877286/attachments/2046061/3427992/PPS_OffSoftMet__2020-05-27.pdf

[ianna]

@jan-kaspar and @ghugo83 - please, update or close this issue. Thanks.

[jan-kaspar]

Thanks @ianna for the reminder! Later today I will try to review the situation and take an action.

[jan-kaspar]

As far as I can recollect now, the suggestions included

• by @ianna : fore reco, we should not store the full XML blob in DB, but a "pre-processed" geometry instead. For that, we have already introduced the new class "PDetGeomDesc" and next we "just" need to do use it (populate DB, updated reco configs).
• by @perrotta : RECO Geometry: CTPPS DetGeomDesc Cleanup #25359 (comment) . Now, looking at the last commit of CTPPS Sim and Reco: Change of paradigm: oldDD and DD4hep in parallel #31383, a1cb064, I see that DetGeomDesc::m_container is a vector, thus I don't think that the suggestion to initialise it with a nullptr is anymore relevant. On the other hand, it still seems missing in the assignment operator:
  

  cmssw/Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc

  Line 63 in a1cb064

  DetGeomDesc& DetGeomDesc::operator=(const DetGeomDesc& ref) {

  
  If you agree, I could address the last issue by a quick PR after CTPPS Sim and Reco: Change of paradigm: oldDD and DD4hep in parallel #31383 is merged.

What do you think?

[jan-kaspar]

... On the other hand, it still seems missing in the assignment operator:

cmssw/Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc

Line 63 in a1cb064

DetGeomDesc& DetGeomDesc::operator=(const DetGeomDesc& ref) {

There was a reason for the missing copy of m_container: the code in CTPPSGeometryESModule::applyAlignments needs a "shallow" copy, i.e. without children. As I agree that this was confusing, I introduce a named method for the shallow copy in #31492 to avoid any possible misunderstanding.

[jan-kaspar]

I think that all items from this issue are now addressed and I suggest that @ianna closes the issue (I cannot do it).

[ianna]

+1