Integrating VectorHits reconstruction for the Phase2 OT #28101
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28101/12096
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
code-checks-rejected
labels
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28101/12097
|
|
A new Pull Request was created by @JanFSchulte (Jan-Frederik Schulte) for master. It involves the following packages: CommonTools/RecoAlgos The following packages do not have a category, yet: RecoLocalTracker/SiPhase2VectorHitBuilder @perrotta, @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
| @@ -257,9 +257,9 @@ void TrackerGeomBuilderFromGeometricDet::buildGeomDet(TrackerGeometry* tracker) | |||
|
|
|||
| } else if (gduTypeName.find("Lower") != std::string::npos) { | |||
| //FIXME::ERICA: the plane builder is built in the middle... | |||
| PlaneBuilderForGluedDet::ResultType plane = gluedplaneBuilder.plane(composed); | |||
| Plane* plane = new Plane(dus->surface()); | |||
There was a problem hiding this comment.
@JanFSchulte - who owns it and when it's cleaned up?
There was a problem hiding this comment.
@ebrondol what is the status of this?
There was a problem hiding this comment.
Please change the FIXME with a comment
//The plane is *not* built in the middle, but on the Lower surface
There was a problem hiding this comment.
Thanks, I have updated it in the latest commit!
There was a problem hiding this comment.
The ownership of this object still is not clear.
There was a problem hiding this comment.
Would changing it to a unique_ptr solve this issue?
Geometry/TrackerGeometryBuilder/src/TrackerGeomBuilderFromGeometricDet.cc
Show resolved
Hide resolved
RecoLocalTracker/SiPhase2VectorHitBuilder/plugins/SiPhase2RecHitMatcherESProducer.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SiPhase2VectorHitBuilder/plugins/SealModules.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SiPhase2VectorHitBuilder/interface/VectorHitBuilderEDProducer.h
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SiPhase2VectorHitBuilder/plugins/SiPhase2RecHitMatcherESProducer.h
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SiPhase2VectorHitBuilder/python/SiPhase2RecHitMatcher_cfi.py
Show resolved
Hide resolved
RecoLocalTracker/SiPhase2VectorHitBuilder/python/SiPhase2VectorHitBuilder_cfi.py
Show resolved
Hide resolved
|
-1 |
|
Comparison is ready Comparison Summary:
|
| @@ -48,18 +50,17 @@ class BaseTrackerRecHit : public TrackingRecHit { | |||
| // verify that hits can share clusters... | |||
| inline bool sameDetModule(TrackingRecHit const& hit) const; | |||
|
|
|||
| bool hasPositionAndError() const final; | |||
| bool hasPositionAndError() const override; | |||
There was a problem hiding this comment.
it looks like these are changed for the needs of a VectorHit.
Shouldn't the logic be roughly the same as with the strip tracker matched hits, which do not need a special treatment for this and other originally "finalized" methods?
There was a problem hiding this comment.
@slava77
Looks like SiStripMatchedRecHit2D just does not have these functions. I can't think of a way to implement this without making the function to final here
| float momentum(const MagneticField* magField); | ||
|
|
||
| ClusterRef lowerCluster() const { return theLowerCluster.cluster_phase2OT(); } | ||
| ClusterRef upperCluster() const { return theUpperCluster.cluster_phase2OT(); } |
There was a problem hiding this comment.
BTW, is the "lower" physically lower or is it e.g. P in PS?
A comment would be nice if it's not physically lower
There was a problem hiding this comment.
The 'lower' is the definition given by the Phase 2 Tracker people to indicate the P of the PS module.
RecoLocalTracker/SiPhase2VectorHitBuilder/src/VectorHitBuilderAlgorithmBase.cc
Show resolved
Hide resolved
RecoLocalTracker/SiPhase2VectorHitBuilder/src/VectorHitBuilderAlgorithmBase.cc
Show resolved
Hide resolved
|
@JanFSchulte I see that you marked all comments received as "resolved", but there was no additional commit submitted here yet with the updates. |
|
@perrotta Yes, I have commits pending to be pushed once I am done with the review. I have marked the issues I have ticked off as resolved mostly just for me to keep track on what I have already addressed. I can un=resolve them for further review once I push the changes. |
| composedDetId = theTopo->stack(gduId[i]); | ||
| StackGeomDet* stackDet = new StackGeomDet(&(*plane), dum, dus, composedDetId); | ||
| StackGeomDet* stackDet = new StackGeomDet(&(*plane), dus, dum, composedDetId); |
There was a problem hiding this comment.
Another question: Is there any point to the &(*plane) construction? Why not just write plane, which is a pointer? If there is a special side effect from this construction, it should be commented.
|
@JanFSchulte are there news or updates of the plans for this PR? |
|
@perrotta This is taking a back seat to some other commitments at the moment. I hope to continue the work on the code later this week for an update early next week. |
|
@JanFSchulte , usual periodic check: is there any update in sight here? |
|
@JanFSchulte if this is desired for the HLT TDR release, it needs to be ready ASAP; otherwise, it will be delayed till 11_1_X |
|
@JanFSchulte : should we "-1" this one, and get back to this PR when you have time to fix it? |
|
@JanFSchulte what is the plan for this PR? Do you expect any activity in the coming days? Or is this moving to a longer timescale? |
|
@fabiocos it looks like this is taking me a bit longer to resolve, so it might be best to close this for the moment and I will come with a new PR in a few weeks. |
|
-1 |
|
Waiting for a new PR as mentioned by @JanFSchulte |
This PR integrates the possibility to reconstruct "Vector Hits" from the 2 hits of the 2S modules in the Phase 2 OT. These hits will have directional as well as positional information and are expected to help especially with seeding for displaced tracks in the OT in Phase 2. The concept is documented extensively in @ebrondol 's PhD thesis: http://cds.cern.ch/record/2308020
Current developments are going to be documented here: https://twiki.cern.ch/twiki/bin/view/CMSPublic/TrackingVectorHits
The strategy for integration of the VHs has been discussed during last week's TRK POG meeting: https://indico.cern.ch/event/847559/contributions/3572893/attachments/1914802/3165228/VectorHits_status_sept25.pdf.
This PR will leave the VH reconstruction deactivated and it not expected to result in any changes in Physics Performance. This has been validated using 1000 ttbar events from a Phase 2 RelVal sample using the D41 detector geometry and workflow 20434.1 in CMSSW_11_0_0_pre9. The output of the full track validation can be found here: https://jschulte.web.cern.ch/jschulte/VH/plots/
As can be seen here https://jschulte.web.cern.ch/jschulte/VH/plots/plots_highPurity/effandfakePtEtaPhi.pdf The performance using this branch is compared to 11_0_0_pre9 out of the box (red vs blue). Comparing the number of hits per track (https://jschulte.web.cern.ch/jschulte/VH/plots/plots_ootb/hitsAndPt.pdf) shows a significant reduction when VH are activated (black markers) as two hits in the 2S modules are merged into one VH. Note that out of the box this reduces the tracking performance.
A follow-up PR will add the necessary tuning of the iterative tracking, including a new pixelLess iteration which will enable displaced track reconstruction for Phase 2. This will include a new era modifier to allow toggling between the current and the proposed configuration. This tuning is in progress but a first version should be available in a couple of weeks.