Fix phase2 tracker association #18354
Conversation
|
A new Pull Request was created by @abbiendi for master. It involves the following packages: SimMuon/MCTruth @civanch, @mdhildreth, @dmitrijus, @cmsbuild, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
Sorry: I wrote several times 910pre5 instead of 900pre5 for the release used to generate the RelVal sample ! Obviously also the legends in the plots are consistently wrong. |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
|
The tests are being triggered in jenkins. |
|
Hi Kevin,
For the moment a private reply, to your questions addressed to
SimTracker/TrackerHitAssociation/src/TrackerHitAssociator.cc.
I had some thought of switching to auto, etc., for some of these
iterators, but let me provide a little background that guides my
approach. This class was written before 2009 when I got involved and
was asked to take it over. I mostly understand it, have rewritten or
added some parts of it in the best style I am aware of (not being a real
c++ master), and have some tools for testing its performance. Other
parts I've not really touched. I take a very conservative approach to
working with code written by others ("first, do no harm").
Most of the items you've flagged here are in code blocks that have one
or more parallel counterparts. For example I copied the
associatePhase2TrackerRecHit function from associatePixelRecHit (that I
inherited), and just made the required changes.
So to follow the spirit of your suggestions I would modify all similar
instances, for consistency. The problem then is, do I have a suite of
tests that I'm sure would catch any mistake I might make. Not being
100% confident of that, I would opt for consistent practice rather than
refining the newest instance by itself. (It's also a question of time;
I gather there's some urgency for Giovanni.)
Regarding cout's, there are certainly some that are just left from
ancient times, that I'd be happy to ditch. Others however work together
to lay a flow trace that I've often returned to when debugging. Maybe
you can tell me how best to handle these: one or more booleans with
hard-coded values? Config booleans with ExistsAs protection? They are
not intended to appear during the course of production, as warnings or
anything else.
I'm open-minded, but cautious. Suggestions welcome.
Bill
On 04/23/2017 03:47 PM, Kevin Pedro wrote:
***@***.**** commented on this pull request.
...
… ------------------------------------------------------------------------
In SimTracker/TrackerHitAssociation/src/TrackerHitAssociator.cc
<#18354 (comment)>:
> @@ -546,6 +534,73 @@ std::vector<SimHitIdpr> TrackerHitAssociator::associateProjectedRecHit(const Pr
return matched_mono;
}
+void TrackerHitAssociator::associatePhase2TrackerRecHit(const Phase2TrackerRecHit1D* rechit,
+ std::vector<SimHitIdpr> & simtrackid,
+ std::vector<simhitAddr>* simhitCFPos) const
+{
+ //
+ // Phase 2 outer tracker associator
+ //
+ DetId detid= rechit->geographicalId();
+ uint32_t detID = detid.rawId();
+
+ edm::DetSetVector<PixelDigiSimLink>::const_iterator isearch = ph2trackerdigisimlink->find(detID);
Type of this variable could be |auto| for simplicity
------------------------------------------------------------------------
In SimTracker/TrackerHitAssociation/src/TrackerHitAssociator.cc
<#18354 (comment)>:
> + if(isearch != ph2trackerdigisimlink->end()) { //if it is not empty
+ edm::DetSet<PixelDigiSimLink> link_detset = (*isearch);
+ Phase2TrackerRecHit1D::CluRef const& cluster = rechit->cluster();
+
+ //check the reference is valid
+
+ if(!(cluster.isNull())){//if the cluster is valid
+ int minRow = (*cluster).firstStrip();
+ int maxRow = (*cluster).firstStrip() + (*cluster).size();
+ int Col = (*cluster).column();
+ // std::cout << " Cluster minRow " << minRow << " maxRow " << maxRow << " column " << Col << std::endl;
+ edm::DetSet<PixelDigiSimLink>::const_iterator linkiter = link_detset.data.begin(), linkEnd = link_detset.data.end();
+ int dsl = 0;
+ std::vector<SimHitIdpr> idcachev;
+ std::vector<simhitAddr> CFposcachev;
+ for( ; linkiter != linkEnd; ++linkiter) {
This could be a range-based for loop over |link_detset.data| (in which
case the various |const_iterator| variables defined above can be removed)
------------------------------------------------------------------------
In SimTracker/TrackerHitAssociation/src/TrackerHitAssociator.cc
<#18354 (comment)>:
> + coord.second == Col ) {
+ // std::cout << " !-> trackid " << linkiter->SimTrackId() << endl;
+ // std::cout << " fraction " << linkiter->fraction() << endl;
+ SimHitIdpr currentId(linkiter->SimTrackId(), linkiter->eventId());
+ if(find(idcachev.begin(),idcachev.end(),currentId) == idcachev.end()){
+ simtrackid.push_back(currentId);
+ idcachev.push_back(currentId);
+ }
+
+ if (simhitCFPos != 0) {
+ //create a vector that contains all the position (in the MixCollection) of the SimHits that contributed to the RecHit
+ //write position only once
+ unsigned int currentCFPos = linkiter->CFposition();
+ unsigned int tofBin = linkiter->TofBin();
+ subDetTofBin theSubDetTofBin = std::make_pair(detid.subdetId(), tofBin);
+ simhit_collectionMap::const_iterator it = SimHitCollMap.find(theSubDetTofBin);
Type of this variable could be |auto| for simplicity
------------------------------------------------------------------------
In SimTracker/TrackerHitAssociation/src/TrackerHitAssociator.cc
<#18354 (comment)>:
> + //
+ DetId detid= rechit->geographicalId();
+ uint32_t detID = detid.rawId();
+
+ edm::DetSetVector<PixelDigiSimLink>::const_iterator isearch = ph2trackerdigisimlink->find(detID);
+ if(isearch != ph2trackerdigisimlink->end()) { //if it is not empty
+ edm::DetSet<PixelDigiSimLink> link_detset = (*isearch);
+ Phase2TrackerRecHit1D::CluRef const& cluster = rechit->cluster();
+
+ //check the reference is valid
+
+ if(!(cluster.isNull())){//if the cluster is valid
+ int minRow = (*cluster).firstStrip();
+ int maxRow = (*cluster).firstStrip() + (*cluster).size();
+ int Col = (*cluster).column();
+ // std::cout << " Cluster minRow " << minRow << " maxRow " << maxRow << " column " << Col << std::endl;
Delete all commented-out |cout| statements
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18354 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfCM61yzssTZ7vZhGWyBj93TlxD8Lehks5ry8cCgaJpZM4M84pB>.
|
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@wmtford it's acceptable to delay the modernization comments I made to a subsequent PR, given the overall state of the code. For the cout statements, if you want to keep them, they should be made into |
|
+1 |
|
+1 |
|
@wmtford : I understand it is up to you to either transform the cout in LogDebug or remove them. I have the impression someone is waiting for this, isn;'t it ? |
|
Hi all, what is still missing that prevents to integrate this PR? Please, consider this is essential for the muon perfomance studies and it is urgent for the Muon TDR. We can't further delay if the remaining issues concerns only few couts, we can fix them later. |
|
Fine, but in general it is not acceptable to delay responses to code review comments until merging is urgent. |
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
This is a bugfix to the muon validation for Phase2 upgrade.
The standard muon validation was silently broken for Phase2 (since 81X, while it worked in 620_SLHCx), due to changes in the Tracker data formats. In particular the outer tracker hits were not associated at all in the TrackerHitAssociator, due to the missing format Phase2TrackerRecHit1D.
This bug was not spotted until now in the standard muon validation as no threshold was asked in the fraction of shared hits. By applying minimum thresholds as in the configuration of the New validation this became apparent.
The TrackerHitAssociator has been kindly updated by Bill Ford (@wmtford). It is also meant to be used for tracker hit validation.
This PR has been tested on SingleMuonPt100 RelVal (with Phase2 conditions 2023 D4T) generated in CMSSW_9_1_0_pre5.
The current validation (OldVal, soon to become obsolete) wrt to plain CMSSW_9_1_0_pre5 is at:
https://cmsdoc.cern.ch/cms/Physics/muon/CMSSW/Performance/RecoMuon/Validation/val/test_NewValidation_fix_Phase2TrackerAssociation/RelValSingleMuPt100Extended_UP2023D4T_noPU/OldVal-fix_Phase2TrackerAssociation_Vs_910pre5/
The comparison of the New validation with respect to the current one is obtained by applying this PR on top of the PR#18290, and is visible here:
https://cmsdoc.cern.ch/cms/Physics/muon/CMSSW/Performance/RecoMuon/Validation/val/test_NewValidation_fix_Phase2TrackerAssociation/RelValSingleMuPt100Extended_UP2023D4T_noPU/fix_Phase2TrackerAssociation-910pre5-NEWVal_Vs_OLDVal/
Comments:
https://cmsdoc.cern.ch/cms/Physics/muon/CMSSW/Performance/RecoMuon/Validation/val/test_NewValidation_fix_Phase2TrackerAssociation/RelValSingleMuPt100Extended_UP2023D4T_noPU/OldVal-fix_Phase2TrackerAssociation_Vs_910pre5/probeTrks.pdf
or: https://cmsdoc.cern.ch/cms/Physics/muon/CMSSW/Performance/RecoMuon/Validation/val/test_NewValidation_fix_Phase2TrackerAssociation/RelValSingleMuPt100Extended_UP2023D4T_noPU/OldVal-fix_Phase2TrackerAssociation_Vs_910pre5/extractedGlobalMuons.pdf
however for Displaced tracks and displaced Global muons there are large effects, see:
https://cmsdoc.cern.ch/cms/Physics/muon/CMSSW/Performance/RecoMuon/Validation/val/test_NewValidation_fix_Phase2TrackerAssociation/RelValSingleMuPt100Extended_UP2023D4T_noPU/OldVal-fix_Phase2TrackerAssociation_Vs_910pre5/displacedTrks.pdf
and: https://cmsdoc.cern.ch/cms/Physics/muon/CMSSW/Performance/RecoMuon/Validation/val/test_NewValidation_fix_Phase2TrackerAssociation/RelValSingleMuPt100Extended_UP2023D4T_noPU/OldVal-fix_Phase2TrackerAssociation_Vs_910pre5/displacedGlobalMuons.pdf
Comparing the NEW vs OLD validation the observed features are all expected for base muon collections
For displaced tracks and global muons the comparison of NEW vs OLD validation demonstrates clearly the improvements of the NEW one, in terms of sharpness. In particular there is a reduced efficiency and increased fake rate in the New validation, as a consequence of the quality cuts. This shows that the recovery of efficiency obtained by this PR on the current (OLD) validation is just recovering low quality tracks. In other words there seems to be a real reconstruction problem with the reconstruction of displaced tracks and displaced global muons. (obviously this has to do with reconstruction, not with its validation)
This bugfix is needed (quite urgently) for muon performance studies for Phase2 TDRs.
@calabria @HuguesBrun @calderona