Overlap Validation for Tracker Alignment #26526
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26526/9398
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-26526/9400 |
|
A new Pull Request was created by @feingoldj for master. It involves the following packages: Alignment/OfflineValidation @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @tlampen, @pohsun, @santocch can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
There was a problem hiding this comment.
Hi @feingoldj
thanks for porting this into cmssw. I have few (mainly stylistic) comments.
Additionally the git commit history is a bit messy and there are transient files which got deleted later in the history (see comment from the bot here: #26526 (comment)).
I am wondering if the commit structure can be improved as there are several distinct updates to the package:
- move the
TkAlStyle.ccfrompluginstomacros - introduce the
OverlapValidationin the all-in-one tool - introduce the
chargeCut_in the OfflineValidation module - save ROOT output for the Z->mm validation (is that really needed)?
Thanks,
| @@ -43,7 +43,7 @@ | |||
| #include <TSystem.h> | |||
| #include <TTimeStamp.h> | |||
| #include <TStopwatch.h> | |||
| //#include "Alignment/OfflineValidation/plugins/TkAlStyle.cc" | |||
| //#include "Alignment/OfflineValidation/macros/TkAlStyle.cc" | |||
There was a problem hiding this comment.
as you moved the TkAlStyle.cc from plugins to macros I am wondering if we could also dump the dependency on:
<use name="rootgraphics"/>
<use name="rootinteractive"/>
we have in https://github.com/cms-sw/cmssw/blob/master/Alignment/OfflineValidation/plugins/BuildFile.xml#L52.
This is a dependency violation and moreover is effectively barring us from submitting condor jobs.
There was a problem hiding this comment.
Right, I remember you mentioned that on mattermost. Done.
| #include "DataFormats/TrackingRecHit/interface/TrackingRecHit.h" | ||
| #include "DataFormats/SiStripDetId/interface/StripSubdetector.h" | ||
| #include "DataFormats/DetId/interface/DetId.h" | ||
| //#include "DataFormats/SiStripDetId/interface/TIBDetId.h" |
There was a problem hiding this comment.
as these DataFormats do not even exist anymore in cmssw, you can remove the commented lines.
| // | ||
|
|
||
|
|
||
| class OverlapValidation : public edm::EDAnalyzer { |
There was a problem hiding this comment.
please avoid using a legacy module.
if this cannot be made multi-thread friendly make it at least a one::EDAnalyzer:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedFrameworkModuleTypes
There was a problem hiding this comment.
I went with one for now. As part of the condor migration maybe someone can make all the validation tools thread safe but it's too much for now.
| // quality cuts on trajectory | ||
| // min. # hits / matched hits | ||
|
|
||
| if ( trajectory.foundHits()<6 ) return; |
There was a problem hiding this comment.
I am wondering if these cuts can be made configurable.
There was a problem hiding this comment.
I made them const variables and moved the hardcoded numbers to the constructor. I didn't propagate to the cfg and all in one tool interface, but at least they're more visible now.
| edm::LogVerbatim("OverlapValidation") << "Invalid"; | ||
| continue; | ||
| } | ||
| if (barrelOnly_ && (subDet==4 || subDet==6) ) return; |
There was a problem hiding this comment.
please use StripSubdetector::TID and StripSubdetector::TEC to improve readability.
| momentum_ = comb1.globalMomentum().mag(); | ||
|
|
||
| // get cluster size | ||
| if (subDet1>2) { //strip |
There was a problem hiding this comment.
please use the
PixelSubdetector::PixelBarrel and PixelSubdetector::PixelEndcap negation instead (or the OR of the 4 Strip subdetIds).
|
|
||
|
|
||
| vector<TrajectoryMeasurement> measurements(trajectory.measurements()); | ||
| for ( vector<TrajectoryMeasurement>::const_iterator itm=measurements.begin(); |
There was a problem hiding this comment.
Not for this one, because we do some iterator math on it down below. But I modified a few of the others.
| } | ||
|
|
||
|
|
||
| if (subDet2<3) { //pixel |
There was a problem hiding this comment.
same comment as a above about using DetId enum.
|
dear @feingoldj , could you please edit the title of your PR : "Overlap validation" can mean many different things in CMS , so please could you please make it more explicit ? thanks |
…2019/cmssw into OverlapValidation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26526/9635
|
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
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 will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
@hroskes @jfan20 @VictordeHavenon @adewit @connorpa @mmusich @gritsan
PR description:
We have developed an Overlap Validation that is adept at detecting the Bowing, Radial, and Z-expansion Weak Modes. It uses the difference between residuals on overlapping modules to characterize the misalignment.
Details can be found here:
Overlap Validation Presentation
PR validation:
The following is the Overlap Validation plot for a radial misalignment with various epsilon values:
We also implemented DMRs separated by track charge that highlights biases that cause Z-expansion in TEC. More information can be found here:
Z-Expansion Update