Skip to content
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.

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

TTStub and TTCluster class porting to CMSSW 10 #21765

Merged
merged 10 commits into from
Jan 28, 2018

Conversation

sviret
Copy link
Contributor

@sviret sviret commented Dec 21, 2017

This branch contains significant updates of the TTStub building algorithm, and a TTCluster association update:

  1. FE inefficiencies: if this option is turned on (default is off), the MPA/CBC and CIC inefficiencies are accounted for based on the event sequence over which stubs are reconstructed. CIC inefficiency is only partially accounted, MPA and CBC are exact.

  2. Hardware bend degradation: bend is encoded on a limited number of bits. Therefore for given stub window tuning all the values cannot be encoded independently and they must be grouped. This process is now done during the Stub building, and the degraded bend can be obtained using the getHardwareBend() method, which is added to the TTStub class. Of course nominal bend is still accessible.

  3. Full parallax correction: within the FE asics, parallax is corrected in half-strips units. However, the exact parallax value can always be retrieved at the backend using the stub position and module radius, and can subsequently be used to improve the bend resolution. This parameter is therefore computed during stub building, and can be retrieved via the method getRealTriggerOffset()

  4. New stub window tuning: stub are built using the new tuning optimized for PU200 events. The default tuning is PU200 TIGHT. For stub tuning documentation, information is available here: https://www.dropbox.com/s/sc1ifhfbcv5pts3/Stub%20windows%20tuning-%20a%20tutorial.pdf?dl=0

  5. Updated TTCluster association scheme:

/// OLD ASSOCIATION SCHEME
/// MC truth
/// Table to define Genuine, Combinatoric and Unknown
///
/// N = number of NULL TP pointers
/// D = number of GOOD TP pointers different from each other
///
/// N / D--> | 0 | 1 | >1
/// ----------------------
/// 0        | U | G | C
/// ----------------------
/// >0       | U | C | C

is replaced by his

/// MC truth
/// Table to define Genuine, Combinatoric and Unknown
///
/// N = number of NULL TP pointers
/// D = number of GOOD TP pointers different from each other
///
/// N / D--> | 0 | 1 | >1 (with 1 TP w/more than 99% of total cluster pT) | >1 (other)
/// -------------------------------------------------------------------
/// 0        | U | G | G                                                  | C
/// -------------------------------------------------------------------
/// >0       | U | G | G                                                  | C
///

With this new association scheme, a cluster made from more than 1 digis, with one digi associated to a low-pt secondary and the other one associated to the primary will be flagged as genuine and associated to the primary TP. Was flagged as combinatoric with the previous association.

IMPORTANT NOTE: This PR does not include any modification of the TTTrack class, as Track Trigger algorithm using it are still in development phase within CMSSW

@cmsbuild cmsbuild changed the base branch from CMSSW_10_0_X to master December 21, 2017 15:34
@cmsbuild
Copy link
Contributor

@sviret, CMSSW_10_0_X branch is closed for direct updates. cms-bot is going to move this PR to master branch.
In future, please use cmssw master branch to submit your changes.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @sviret (Seb Viret) for master.

It involves the following packages:

DataFormats/L1TrackTrigger
L1Trigger/TrackTrigger
SimTracker/TrackTriggerAssociation

@cmsbuild, @civanch, @mdhildreth, @nsmith-, @rekovic, @kpedro88, @thomreis can you please review it and eventually sign? Thanks.
@kreczko, @makortel, @sdevissc, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @ebrondol, @threus, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@kpedro88
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 21, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25248/console Started: 2017/12/21 20:26

@@ -80,6 +88,8 @@ TTStub< T >::TTStub()
theDetId = 0;
theDisplacement = 999999;
theOffset = 0;
theRealOffset = 0;
theHardwareBend = 999999;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the "999999" value is used in multiple places, it should be a const member of the class (e.g. defaultHardwareBend)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// Loop over all the tracker elements

for (auto gd=theTrackerGeom->dets().begin(); gd != theTrackerGeom->dets().end(); gd++)
Copy link
Contributor

Choose a reason for hiding this comment

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

range-based loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
else if (detid.subdetId()==StripSubdetector::TID)
{
if (tTopo->tidWheel(detid)<=2 && tTopo->tidRing(detid)<=4) is10G_PS = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these magic numbers be obtained from some geometry/topology class? Otherwise, should be made into descriptive constants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, numbers have been passed as joboptions

const PixelGeomDetUnit* pix0 = dynamic_cast< const PixelGeomDetUnit* >( det0 );
const PixelTopology* top0 = dynamic_cast< const PixelTopology* >( &(pix0->specificTopology()) );
const int chipSize = 2 * top0->rowsperroc(); /// Need to find ASIC size in half-strip units
//const GeomDetUnit* det0 = theTrackerGeom->idToDetUnit( lowerDetid );
Copy link
Contributor

Choose a reason for hiding this comment

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

delete commented-out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if ( moduleStubs.find( chip ) == moduleStubs.end() ) /// Already a stub for this ASIC?
MeasurementPoint mp0 = tempTTStub.getClusterRef(0)->findAverageLocalCoordinates();
int seg = static_cast<int>(mp0.y());
if (isPS) seg = seg/16;
Copy link
Contributor

Choose a reason for hiding this comment

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

use consistent indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps done...

throw cms::Exception("DegradeBend: logic error with odd numbers");
}

for (unsigned int i = 0; i < numSmallGroups/2; i++) groups.push_back(inSmallGroup);
Copy link
Contributor

Choose a reason for hiding this comment

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

reserve() again

@@ -146,6 +163,12 @@ bool TTClusterAssociationMap< T >::isGenuine( edm::Ref< edmNew::DetSetVector< TT
unsigned int goodDifferentTPs = 0;
std::vector< const TrackingParticle* > tpAddressVector;

std::vector<float> tp_mom;

tp_mom.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to clear vector right after creating it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


if (tp_tot==0) return false;

for ( unsigned int itp = 0; itp < theseTrackingParticles.size(); itp++ )
Copy link
Contributor

Choose a reason for hiding this comment

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

range-based loop w/ const auto&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


/// Count how many different TrackingParticle there are
std::sort( tpAddressVector.begin(), tpAddressVector.end() );
tpAddressVector.erase( std::unique( tpAddressVector.begin(), tpAddressVector.end() ), tpAddressVector.end() );
goodDifferentTPs = tpAddressVector.size();
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

delete commented-out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// GENUINE means no NULLs and only one good TP
return ( nullTPs == 0 && goodDifferentTPs == 1 );
//return ( nullTPs == 0 && goodDifferentTPs == 1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

delete commented-out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@cmsbuild
Copy link
Contributor

-1

Tested at: 4d1c103

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21765/25248/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
1325.7 step2

runTheMatrix-results/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017/step2_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017.log

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@sviret
Copy link
Contributor Author

sviret commented Jan 26, 2018

Not sure that this error is related to the PR itself.

@boudoul
Copy link
Contributor

boudoul commented Jan 26, 2018

There are not, if you look at the log, this is an error when trying to open a file ... Seems other PR are affected too

@fabiocos
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 27, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25681/console Started: 2018/01/27 19:09

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21765/25681/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-21765/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2464350
  • DQMHistoTests: Total failures: 148
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2464033
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.970000000052 KiB( 22 files compared)
  • Checked 110 log files, 9 edm output root files, 26 DQM output files

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 46be6bb into cms-sw:master Jan 28, 2018
@rekovic
Copy link
Contributor

rekovic commented Feb 12, 2018

@sviret, @boudoul
I guess we need to backport this to 93x if we want PU200 tuned TTStubs and correct TrackTriggerAssociatorClustersStubs for the Tracker geometry T5 used in CMSSW_9_3_5.
If that is correct, would it be possible to have the backport?
(attn @kpedro88)

@kpedro88
Copy link
Contributor

@rekovic since this PR only affects trigger, it should be acceptable to backport it to 9_3_X and make a 9_3_6 release for trigger study production.

@rekovic
Copy link
Contributor

rekovic commented Feb 12, 2018

@kpedro88 Thx.

@kpedro88
Copy link
Contributor

@rekovic is this backport going to happen?

@rekovic
Copy link
Contributor

rekovic commented Feb 23, 2018

@kpedro88
Was waiting for Tracking, but here it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants