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

Tracking vertex fix #4700

Merged
merged 10 commits into from
Jul 24, 2014
Merged

Tracking vertex fix #4700

merged 10 commits into from
Jul 24, 2014

Conversation

dnowatsc
Copy link

Added the SimVertexs and refs to HepMC::GenVertexs to TrackingVertex in TrackingTruthAccumulator.cc and introduced two parameters in the trackingTruthProducer_cfi.py file, my changes are summarized in the last commit 35583bf. I also documented my changes in the source file, if they are insufficient please don't hesitate to send me an e-mail, also see my post in the Tracking hn.

The deletion of SimTracker/TrackHistory/plugins/TrackCategoriesAnalyzer.cc is more of an accident, I modified it to test my changes to the other files with it and removed it from the staging area in git so these changes will not be pushed; so if possible, please ignore this.

Dominik Nowatschin added 6 commits July 16, 2014 15:17
…eateTrackingVertex'; for GenVertexRef used same approach from 'TrackingTruthProducer' in 5X so add all GenVertexs within a distance of 0.003 cm to TrackingVertex
…al::getByLabel()' is already bypassed when calling 'TrackingTruthAccumulator::accumulate'; also 'vertexDistanceCut' and 'HepMCProductLabel' are added to SimGeneral/MixingModule/python/trackingTruthProducer_cfi.py as parameters
	* in 'createTrackingVertex', add SimVertex to TrackingVertex which is created from the SimVertex ('returnValue.addG4Vertex(simVertex)')
	* also add ref to HepMC::GenVertexs in createTrackingVertex if it is a signal event; add all nearby GenVertexs (copied from old 'TrackingTruthProducer')
	* add handle to edm::HepMCProduct already in 'TrackingTruthAccumulator::accumulate' (and not in 'accumulateEvent') as 'event.getByLabel()' with it doesn't work with PileUpEventPrincipal
	* add parameter 'vertexDistanceCut' in SimGeneral/MixingModule/python/trackingTruthProducer_cfi.py to control this behaviour (set to 0.003 cm by default)
	* also add 'HepMCProductLabel' parameter in trackingTruthProducer_cfi.py
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dnowatsc for CMSSW_7_2_X.

Tracking vertex fix

It involves the following packages:

.gitignore
SimGeneral/MixingModule
SimGeneral/TrackingAnalysis
SimTracker/TrackHistory

@civanch, @Dr15Jones, @mdhildreth, @cmsbuild, @nclopezo, @Degano, @ktf can you please review it and eventually sign? Thanks.
@wmtan, @GiacomoSguazzoni, @rovere, @VinInn, @cerati, @threus, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@VinInn
Copy link
Contributor

VinInn commented Jul 18, 2014

remove .gitignore

@VinInn
Copy link
Contributor

VinInn commented Jul 18, 2014

please fix SimTracker/TrackHistory/plugins/TrackCategoriesAnalyzer.cc

cp $CMSSW_RELEASE_BASE/src/SimTracker/TrackHistory/plugins/TrackCategoriesAnalyzer.cc .
git add SimTracker/TrackHistory/plugins/TrackCategoriesAnalyzer.cc
git commit ...

etc

Vector genPosition(rawPosition.x()/10.0, rawPosition.y()/10.0, rawPosition.z()/10.0);

double distance = sqrt( (tvPosition - genPosition).mag2() );

Copy link
Contributor

Choose a reason for hiding this comment

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

*0.1 is faster then /10.0

auto distance2 = (tvPosition - genPosition).mag2() ;
if (distance2 < vertexDistanceCut_*vertexDistanceCut_)
is faster
(redefine vertexDistanceCut_ as vertexDistanceCut2_ even better

@dnowatsc
Copy link
Author

I don't really get what you mean in the last comment, is this just a naming convention (so vertexDistanceCut2_ because only the square of its value is used)?

@VinInn
Copy link
Contributor

VinInn commented Jul 18, 2014

vertexDistanceCut2_ because only the square of its value is used
yes, to make sure in future people do not get confused

…esAnalyzer.cc plus a few changes in TrackingTruthAccumulator
@cmsbuild
Copy link
Contributor

Pull request #4700 was updated. @cmsbuild, @civanch, @Degano, @mdhildreth, @nclopezo can you please check and sign again.


auto distance2 = (tvPosition - genPosition).mag2();

if (distance2 < vertexDistanceCut2_*vertexDistanceCut2_)
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I was not precise. I meant
vertexDistanceCut2_(vertexDistanceCut*vertexDistanceCut)
and
if (distance2 < vertexDistanceCut2_)

Copy link
Author

Choose a reason for hiding this comment

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

So you mean putting it like this in the constructor i.e. somewhere around line 520?

@cmsbuild
Copy link
Contributor

Pull request #4700 was updated. @cmsbuild, @civanch, @Degano, @mdhildreth, @nclopezo can you please check and sign again.

@cmsbuild
Copy link
Contributor

-1
Tested at: a11c8bb
When I ran the RelVals I found an error in the following worklfows:
5.1 step1

runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log
----- Begin Fatal Exception 18-Jul-2014 18:47:17 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=MixingModule label='mix'
Exception Message:
MissingParameter: Parameter 'vertexDistanceCut' not found.
----- End Fatal Exception -------------------------------------------------

401.0 step1

runTheMatrix-results/401.0_TTbarNewMix+TTbarFSPU2+HARVESTFS/step1_TTbarNewMix+TTbarFSPU2+HARVESTFS.log
----- Begin Fatal Exception 18-Jul-2014 18:48:21 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=MixingModule label='mix'
Exception Message:
MissingParameter: Parameter 'vertexDistanceCut' not found.
----- End Fatal Exception -------------------------------------------------

50101.0 step2

runTheMatrix-results/50101.0_SingleMuPt10+SingleMuPt10FSIdINPUT+SingleMuPt10FS_ID/step2_SingleMuPt10+SingleMuPt10FSIdINPUT+SingleMuPt10FS_ID.log
----- Begin Fatal Exception 18-Jul-2014 18:56:13 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=MixingModule label='mix'
Exception Message:
MissingParameter: Parameter 'vertexDistanceCut' not found.
----- End Fatal Exception -------------------------------------------------

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

@lveldere
Copy link
Contributor

I asked Dominik in private for a few changes to avoid configuration duplication between FastSim and FullSim, I will approve as soon as it is clear whether Dominik can do this or not.

…rSelection_cfi.py to reduce overlaps between FastSim and FullSim
@lveldere
Copy link
Contributor

+1
thanks a lot Dominik!

@cmsbuild
Copy link
Contributor

Pull request #4700 was updated. @cmsbuild, @civanch, @Degano, @mdhildreth, @nclopezo can you please check and sign again.

@cmsbuild
Copy link
Contributor

@civanch
Copy link
Contributor

civanch commented Jul 24, 2014

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

ktf added a commit that referenced this pull request Jul 24, 2014
@ktf ktf merged commit a54ec31 into cms-sw:CMSSW_7_2_X Jul 24, 2014
@cmsbuild
Copy link
Contributor

@cerati
Copy link
Contributor

cerati commented Aug 18, 2014

validation report by @avartak spotted differences possibly due to this PR (https://hypernews.cern.ch/HyperNews/CMS/get/relval/3099/49.html)
Where can I find the test results? Links appear to be broken

@cerati
Copy link
Contributor

cerati commented Aug 18, 2014

Sorry, please ignore the request above - differences in FASTSIM are indeed expected (no difference in FULLSIM)

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

7 participants