Skip to content

Update of mkFit for 12_1_0_pre2 #34921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cmsbuild merged 6 commits into from
Aug 25, 2021
Merged

Update of mkFit for 12_1_0_pre2 #34921

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c9dd8e6
Enable mkFit by default only for InitialStepPreSplitting, InitialStep…
mmasciov Aug 16, 2021
8d21af8
Enable usage of SiStrip quality DB information by default for mkFit
mmasciov Aug 16, 2021
fbbdad2
Restrict SiStrip DB information to invalid APVs if only a part of the…
mmasciov Aug 16, 2021
2b3e7b3
From code checks/format
mmasciov Aug 16, 2021
30567c1
Update README with description of mkFit-specific parameters
mmasciov Aug 16, 2021
91d1caf
Use constants and derive nAPVs from the strip topology, and add deps …
mmasciov Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Configuration/ProcessModifiers/python/trackingMkFit_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
trackingMkFitCommon,
trackingMkFitInitialStepPreSplitting,
trackingMkFitInitialStep,
trackingMkFitLowPtQuadStep,
# trackingMkFitLowPtQuadStep, # to be enabled later
trackingMkFitHighPtTripletStep,
trackingMkFitLowPtTripletStep,
# trackingMkFitLowPtTripletStep, # to be enabled later
trackingMkFitDetachedQuadStep,
# trackingMkFitDetachedTripletStep, # to be enabled later
# trackingMkFitPixelPairStep, # to be enabled later
# trackingMkFitMixedTripletStep, # to be enabled later
trackingMkFitPixelLessStep,
trackingMkFitTobTecStep,
# trackingMkFitPixelLessStep, # to be enabled later
# trackingMkFitTobTecStep, # to be enabled later
)
4 changes: 3 additions & 1 deletion RecoTracker/MkFit/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ $ runTheMatrix.py -l <workflow(s)> --apply 2 --command "--procModifiers tracking
* *maxCandsPerSeed:* maximum number of concurrent track candidates per given seed
* *maxHolesPerCand:* maximum number of allowed holes on a candidate
* *maxConsecHoles:* maximum number of allowed consecutive holes on a candidate
* *chi2Cut:* chi2 cut for accepting a new hit
* *chi2Cut_min:* minimum chi2 cut for accepting a new hit
* *chi2CutOverlap:* chi2 cut for accepting an overlap hit
* *pTCutOverlap:* pT cut below which the overlap hits are not picked up

Expand Down Expand Up @@ -81,3 +81,5 @@ $ runTheMatrix.py -l <workflow(s)> --apply 2 --command "--procModifiers tracking
* *c_dp_sf:* additional scaling factor for dphi cut
* *c_dq_[012]:* dr (endcap) / dz (barrel) selection window cut (= [0]*1/pT + [1]*std::fabs(theta-pi/2) + [2])
* *c_dq_sf:* additional scaling factor for dr (endcap) / dz (barrel) cut
* *c_c2_[012]:* chi2 cut for accepting new hit (= [0]*1/pT + [1]*std::fabs(theta-pi/2) + [2])
* *c_c2_sf:* additional scaling factor for chi2 cut
62 changes: 51 additions & 11 deletions RecoTracker/MkFit/plugins/MkFitEventOfHitsProducer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,21 @@
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"

#include "CalibFormats/SiStripObjects/interface/SiStripQuality.h"
#include "CalibTracker/Records/interface/SiStripQualityRcd.h"

#include "DataFormats/TrackerCommon/interface/TrackerDetSide.h"
#include "DataFormats/TrackingRecHit/interface/TrackingRecHit.h"

#include "Geometry/CommonTopologies/interface/StripTopology.h"
#include "Geometry/TrackerGeometryBuilder/interface/TrackerGeometry.h"

#include "RecoTracker/MkFit/interface/MkFitClusterIndexToHit.h"
#include "RecoTracker/MkFit/interface/MkFitEventOfHits.h"
#include "RecoTracker/MkFit/interface/MkFitGeometry.h"
#include "RecoTracker/MkFit/interface/MkFitHitWrapper.h"
#include "RecoTracker/Record/interface/TrackerRecoGeometryRecord.h"

#include "CalibFormats/SiStripObjects/interface/SiStripQuality.h"
#include "CalibTracker/Records/interface/SiStripQualityRcd.h"
#include "Geometry/TrackerGeometryBuilder/interface/TrackerGeometry.h"
#include "DataFormats/TrackerCommon/interface/TrackerDetSide.h"

// mkFit includes
#include "mkFit/HitStructures.h"
#include "mkFit/MkStdSeqs.h"
Expand Down Expand Up @@ -66,7 +68,7 @@ void MkFitEventOfHitsProducer::fillDescriptions(edm::ConfigurationDescriptions&

desc.add("pixelHits", edm::InputTag{"mkFitSiPixelHits"});
desc.add("stripHits", edm::InputTag{"mkFitSiStripHits"});
desc.add("useStripStripQualityDB", false)->setComment("Use SiStrip quality DB information");
desc.add("useStripStripQualityDB", true)->setComment("Use SiStrip quality DB information");

descriptions.addWithDefaultLabel(desc);
}
Expand All @@ -85,14 +87,52 @@ void MkFitEventOfHitsProducer::produce(edm::StreamID iID, edm::Event& iEvent, co
const auto& trackerGeom = iSetup.getData(geomToken_);
const auto& badStrips = siStripQuality.getBadComponentList();
for (const auto& bs : badStrips) {
const auto& surf = trackerGeom.idToDet(DetId(bs.detid))->surface();
const DetId detid(bs.detid);
const auto& surf = trackerGeom.idToDet(detid)->surface();
bool isBarrel = (mkFitGeom.topology()->side(detid) == static_cast<unsigned>(TrackerDetSide::Barrel));
const auto ilay = mkFitGeom.mkFitLayerNumber(detid);
deadvectors[ilay].push_back({surf.phiSpan().first,
surf.phiSpan().second,
(isBarrel ? surf.zSpan().first : surf.rSpan().first),
(isBarrel ? surf.zSpan().second : surf.rSpan().second)});
const auto q1 = isBarrel ? surf.zSpan().first : surf.rSpan().first;
const auto q2 = isBarrel ? surf.zSpan().second : surf.rSpan().second;
if (bs.BadModule)
deadvectors[ilay].push_back({surf.phiSpan().first, surf.phiSpan().second, q1, q2});
else { //assume that BadApvs are filled in sync with BadFibers
auto const& topo = dynamic_cast<const StripTopology&>(trackerGeom.idToDet(detid)->topology());
int firstApv = -1;
int lastApv = -1;

auto addRangeAPV = [&topo, &surf, &q1, &q2](int first, int last, mkfit::DeadVec& dv) {
auto const firstPoint = surf.toGlobal(topo.localPosition(first * 128));
auto const lastPoint = surf.toGlobal(topo.localPosition((last + 1) * 128));
float phi1 = firstPoint.phi();
float phi2 = lastPoint.phi();
if (reco::deltaPhi(phi1, phi2) > 0)
std::swap(phi1, phi2);
LogTrace("SiStripBadComponents")
<< "insert bad range " << first << " to " << last << " " << phi1 << " " << phi2;
dv.push_back({phi1, phi2, q1, q2});
};

for (int apv = 0; apv < 6; ++apv) {
Copy link
Contributor

@jpata jpata Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might the apv max number be a named constant?

Copy link
Contributor

@slava77 slava77 Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pieterdavid @mmusich
do we have max APVs and n strips per APV formalized in common constants somewhere?
I tried to git grep, but all that I found was hardcoded 6 and 128.

Copy link
Contributor

@mmusich mmusich Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about modules with less than 6 APVs?

Copy link
Contributor

@mmusich mmusich Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have max APVs and n strips per APV formalized in common constants somewhere?

they are (at least) in this file: https://github.com/cms-sw/cmssw/blob/master/CalibTracker/SiStripCommon/data/SiStripDetInfo.dat.
Or you can do something like:

      unsigned short nApvs = stripDet->specificTopology().nstrips() / 128;

128 is univeral for all modules, I am not sure there's a constant somewhere.

Copy link
Contributor

@pieterdavid pieterdavid Aug 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the constants are also here (but they aren't used much in the strips code): https://github.com/cms-sw/cmssw/blob/master/DataFormats/SiStripCommon/interface/ConstantsForHardwareSystems.h#L38-L43 (6 as max number of APVs indirectly, as 2 per optical link * max 3 of these per module - it may be worth adding)

Copy link
Contributor

@slava77 slava77 Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about modules with less than 6 APVs?

it shouldn't matter here, if the BadApvs is filled correctly.

Copy link
Contributor Author

@mmasciov mmasciov Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in commit 91d1caf

const bool isBad = bs.BadApvs & (1 << apv);
if (isBad)
LogTrace("SiStripBadComponents") << "bad apv " << apv << " on " << bs.detid;
if (isBad) {
if (lastApv == -1) {
Copy link
Contributor

@jpata jpata Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not like this? Is there something special about LogTrace?

Suggested change
if (isBad)
LogTrace("SiStripBadComponents") << "bad apv " << apv << " on " << bs.detid;
if (isBad) {
if (lastApv == -1) {
if (isBad) {
LogTrace("SiStripBadComponents") << "bad apv " << apv << " on " << bs.detid;
if (lastApv == -1) {

Copy link
Contributor

@slava77 slava77 Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the suggestion is OK, I was moving these lines a few too many times.
LogTrace does not have additional header/footer message formatting and is easier to parse than LogDebug.

Copy link
Contributor Author

@mmasciov mmasciov Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in commit 91d1caf

firstApv = apv;
lastApv = apv;
} else if (lastApv + 1 == apv)
lastApv++;

if (apv == 5)
addRangeAPV(firstApv, lastApv, deadvectors[ilay]);
} else if (firstApv != -1) {
addRangeAPV(firstApv, lastApv, deadvectors[ilay]);
//and reset
firstApv = -1;
lastApv = -1;
}
}
}
}
mkfit::StdSeq::LoadDeads(*eventOfHits, deadvectors);
}
Expand Down