Pending items to be addressed after the integration of the first part of the MTD tracking/geometry PR in CMSSW #24452

perrotta · 2018-09-04T07:42:05Z

The following issues raised during the review of #24285 were not addressed in that PR. The idea is to integrate now the large changes included in that pull request, so that development can continue more easily in the IBs. All the issue found need to be fixed after that PR get merged in the CMSSW release, however.

From MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment):
- Correct navigation by following the prescription of the Tracking POG and the outcome of the discussion in that forum
From MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment):
- RecoMTD/DetLayers/src/MTDRingForwardDoubleLayer.cc: refactor compatibleDets() code to common location (MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment))
- RecoMTD/DetLayers/src/MTDRingForwardDoubleLayer.cc: modernize ReferenceCountingPointer (MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment))
- RecoMTD/Navigation/src/MTDNavigableLayer.cc etc.: make into a common class if no differences necessary for MTD usage (MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment))
- DataFormats/FTLRecHit/src/FTLTrackingRecHit.cc: extend trackerHitRTTI for this new class, rather than using typeid (MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment))
- Geometry/MTDNumberingBuilder/interface/GeometricTimingDetExtra.h: trailing underscore for data members (MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment))
- Geometry/MTDNumberingBuilder/interface/CmsMTDStringToEnum.h: trailing underscore for data members (MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment))
From MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285 (comment)
- Remove all duplicates and mirroring of the classes moved to TrackingTools/DetLayers

@lgray @kpedro88 @VinInn @slava77 @ianna

The text was updated successfully, but these errors were encountered:

perrotta · 2018-09-04T07:42:22Z

assign reconstruction, geometry

cmsbuild · 2018-09-04T07:42:55Z

New categories assigned: geometry,reconstruction

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

cmsbuild · 2018-09-04T07:42:57Z

A new Issue was created by @perrotta .

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

cms-bot commands are listed here

kpedro88 · 2018-09-04T14:14:07Z

assign upgrade

cmsbuild · 2018-09-04T14:14:19Z

New categories assigned: upgrade

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

kpedro88 · 2018-10-16T14:25:25Z

We might consider downgrading the warnings here to LogInfo, otherwise they will be printed in every pre-Phase2 workflow for the next N years:

cmssw/Geometry/GlobalTrackingGeometryBuilder/plugins/GlobalTrackingGeometryESProducer.cc

Lines 51 to 61 in 94c2443

    
           try { 
        
             edm::ESHandle<MTDGeometry> mtdH; 
        
             record.getRecord<MTDDigiGeometryRecord>().get(mtdH); 
        
             if(mtdH.isValid()) { 
        
               mtd = mtdH.product(); 
        
             } else { 
        
               LogWarning("GeometryGlobalTrackingGeometryBuilder") << "No MTD geometry is available."; 
        
             } 
        
           } catch (edm::eventsetup::NoRecordException<MTDDigiGeometryRecord>& e){ 
        
             LogWarning("GeometryGlobalTrackingGeometryBuilder") << "No MTDDigiGeometryRecord is available.";     
        
           }

kpedro88 · 2018-11-01T19:00:39Z

loginfo and RTTI items addressed by #25063

others remain to be addressed later

kpedro88 · 2019-02-05T02:15:00Z

ianna · 2020-10-22T09:10:32Z

@fabiocos - please, check if there are any outstanding issues. Thanks!

fabiocos · 2020-10-22T10:34:16Z

@ianna @kpedro88 yes, I had kept this link in my folder to have a look at it, I will check for a cleaning when the basics of the ongoing PR are done

fabiocos · 2020-11-04T16:45:12Z

#32026 addresses some of the comments listed by @perrotta at the beginning of this issue in the Geometry and RecoMTD/DetLayers domains. There are two other issues there not managed for the time being:

the refactorization of compatibleDets in the MTDRingForwardDoubleLayer class, with the problem being the same in the newly introduced MTDSectorForwardDoubleLayer class. Here honestly I wonder whether it is worthwhile to spend effort just to avoid having in two or three classes a similar loop of just a few lines, based on the example provided in the base class (but with a dummy implementation of groupedCompatibleDetsV). One could think to move all the relevant code for the derived class method inside a derived implementation of the dummy method, but I doubt this is really clean;
I am not sure which kind of modernization of ReferenceCountingPointer is desired, may be getting rid of boost? In any case this goes well beyond MTD code, and it is a project in itself, unless I am misinterpreting the point.

Furthermore @ianna was suggesting to replace within MTDDetId data format enum with enum class. This can be done, but it requires updates in a number of different places around the code, and the way I see it it would imply using static_cast within the boolean bit operations with masks, and as far as I can see there is no other such case within CMSSW. Before proposing such an update I'd like to be sure this is ok for other experts (@makortel any comment?)

makortel · 2020-11-04T17:36:52Z

In my opinion if the use of enum class leads to substantial use of static_cast to access the numerical value, it could be easier to stay with plain enum.

perrotta · 2020-11-04T17:38:19Z

Thank you @fabiocos,
Since in this issue there is also an action which relates to the RecoMuon code, before forgetting about it I provided a (trivial) fix with #32029

fabiocos · 2020-11-05T11:43:48Z

@makortel thanks for the comment, in operations like those implied by https://github.com/cms-sw/cmssw/blob/master/DataFormats/ForwardDetId/interface/BTLDetId.h#L76 an explicit type cast is required il enum class is needed as far as I can see, just make a trivial test

12:37 mb-fc-03 534> cat testEnum.cpp 
#include <iostream>

int main() {

  enum pippo { aa = 1, bb = 2, cc = 3 };
  enum class ciccio : uint32_t { AA = 1, BB = 2, CC = 3 };

  const uint32_t mask = 0x2;

  std::cout << "Plain enum operation: " << (pippo::aa & mask) << " " << (pippo::bb & mask) << " " << (pippo::cc & mask) << std::endl;

  std::cout << "Class enum operation: " << (static_cast<uint32_t>(ciccio::AA) & mask) << " " << ((uint32_t)ciccio::BB & mask) << " " << (ciccio::CC & mask) << std::endl;

  return 0;

}

giving (on Mac with clang 11, but this is irrelevant here I believe)

12:41 mb-fc-03 535> g++ -std=c++11 testEnum.cpp 
testEnum.cpp:12:149: error: invalid operands to binary expression ('ciccio' and 'const uint32_t' (aka 'const unsigned int'))
  std::cout << "Class enum operation: " << (static_cast<uint32_t>(ciccio::AA) & mask) << " " << ((uint32_t)ciccio::BB & mask) << " " << (ciccio::CC & mask) << std::endl;
                                                                                                                                         ~~~~~~~~~~ ^ ~~~~
1 error generated.

srimanob · 2021-04-11T11:53:21Z

Hi @fabiocos
Would you mind summarising what still need to follow up with this ticket? Thanks very much.

fabiocos · 2021-04-11T15:19:53Z

@srimanob this issue is always in my inbox since 2018, I need to review with calm what possibly still applies as there were so many updates over time.

fabiocos · 2021-05-10T15:50:28Z

fabiocos · 2021-05-12T08:26:44Z

@srimanob as far as I can see, all the issues in the list by Kevin are addressed in #33680 .

srimanob · 2021-05-12T08:34:30Z

Thanks very much, @fabiocos
When #33680 is merged, this issue can be closed.

fabiocos · 2021-05-12T08:37:22Z

@perrotta I believe that most of the original items of yours were already addressed, and for the few remaining #24452 (comment) I already commented.

perrotta · 2021-05-12T10:04:01Z

@perrotta I believe that most of the original items of yours were already addressed, and for the few remaining #24452 (comment) I already commented.

Ok, thank you Fabio.

About "modernization of ReferenceCountingPointer", this was proposed by @lgray in the comment at L74 of RecoMTD/DetLayers/src/MTDRingForwardDoubleLayer.cc in #24285: I agree that this goes beyond the MTD domain, since it touches all tracking geometries in CMSSW.

MTD reconstruction: code cleaning for old comments (issue #24452)

fabiocos · 2021-05-17T10:00:24Z

Now that #33680 has been merged, I would consider this issue as basically solved, accounting for the comments above.

perrotta · 2021-05-17T10:04:45Z

+reconstruction

cmsbuild added geometry-pending pending-signatures reconstruction-pending labels Sep 4, 2018

perrotta mentioned this issue Sep 4, 2018

MTD Tracking Part 1 - Topology, Geometry, and Tracking Geometry #24285

Merged

cmsbuild added the upgrade-pending label Sep 4, 2018

lgray mentioned this issue Sep 4, 2018

Extend TrackBase and Track to include time and velocity #24465

Merged

fabiocos mentioned this issue Nov 4, 2020

MTD geometry and reco: code cleaning #32026

Merged

perrotta mentioned this issue Nov 4, 2020

Replace mirrored finders in RecoMuon from TrackingTools/DetLayer with the original ones #32029

Merged

fabiocos mentioned this issue May 10, 2021

MTD reconstruction: code cleaning for old comments (issue #24452) #33680

Merged

cmsbuild added a commit that referenced this issue May 17, 2021

Merge pull request #33680 from fabiocos/fc-mtdclean

b387c16

MTD reconstruction: code cleaning for old comments (issue #24452)

cmsbuild added reconstruction-approved and removed reconstruction-pending labels May 17, 2021

perrotta closed this as completed May 17, 2021

This was referenced May 17, 2021

Remove use of root_numpy in DQM BinByBin tool #33751

Merged

Remove dependencies in FWCore BuildFiles #33755

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pending items to be addressed after the integration of the first part of the MTD tracking/geometry PR in CMSSW #24452

Pending items to be addressed after the integration of the first part of the MTD tracking/geometry PR in CMSSW #24452

perrotta commented Sep 4, 2018

perrotta commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

kpedro88 commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

kpedro88 commented Oct 16, 2018

kpedro88 commented Nov 1, 2018

kpedro88 commented Feb 5, 2019

ianna commented Oct 22, 2020

fabiocos commented Oct 22, 2020

fabiocos commented Nov 4, 2020

makortel commented Nov 4, 2020

perrotta commented Nov 4, 2020

fabiocos commented Nov 5, 2020

srimanob commented Apr 11, 2021

fabiocos commented Apr 11, 2021

fabiocos commented May 10, 2021 •

edited

fabiocos commented May 12, 2021

srimanob commented May 12, 2021

fabiocos commented May 12, 2021

perrotta commented May 12, 2021

fabiocos commented May 17, 2021

perrotta commented May 17, 2021

Pending items to be addressed after the integration of the first part of the MTD tracking/geometry PR in CMSSW #24452

Pending items to be addressed after the integration of the first part of the MTD tracking/geometry PR in CMSSW #24452

Comments

perrotta commented Sep 4, 2018

perrotta commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

kpedro88 commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

kpedro88 commented Oct 16, 2018

kpedro88 commented Nov 1, 2018

kpedro88 commented Feb 5, 2019

ianna commented Oct 22, 2020

fabiocos commented Oct 22, 2020

fabiocos commented Nov 4, 2020

makortel commented Nov 4, 2020

perrotta commented Nov 4, 2020

fabiocos commented Nov 5, 2020

srimanob commented Apr 11, 2021

fabiocos commented Apr 11, 2021

fabiocos commented May 10, 2021 • edited

fabiocos commented May 12, 2021

srimanob commented May 12, 2021

fabiocos commented May 12, 2021

perrotta commented May 12, 2021

fabiocos commented May 17, 2021

perrotta commented May 17, 2021

fabiocos commented May 10, 2021 •

edited