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

Triplet check in FastSim #14083

Closed
wants to merge 22 commits into from
Closed

Triplet check in FastSim #14083

wants to merge 22 commits into from

Conversation

a-kapoor
Copy link
Contributor

@a-kapoor a-kapoor commented Apr 15, 2016

FastSim now checks if a triplet is compatible with tracking region constraints.

The validation plots are available here : http://test-folders.web.cern.ch/test-Folders/plots_TripletFull_11April/

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Ansh0712 (Anshul Kapoor) for CMSSW_8_1_X.

It involves the following packages:

FastSimulation/Tracking
RecoPixelVertexing/PixelTriplets
RecoTracker/TkSeedGenerator

@civanch, @lveldere, @cvuosalo, @ssekmen, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @istaslis, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mschrode, @matt-komm, @gpetruc, @cerati, @dgulhan this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

const RecHitsSortedInPhi* thm=new RecHitsSortedInPhi (thirdHits, selectedTrackingRegion->origin(), thirdLayer);
if(pixelTripletGeneratorPtr){
OrderedHitTriplets Tripletresult;
pixelTripletGeneratorPtr->hitTriplets(*selectedTrackingRegion,Tripletresult,es,*hitDoublets,&thm,thirdLayerDetLayer,1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you use the hitDoublets again to retrieve the information. You assume that together with the third hit these form a triplet that satisfies the seeding layers. I think there can be cases where this is not true, e.g. if you have two hits for the first layer, then you get two doublets where the first one is overridden by the next one and hence the two hits in the doublets are not identical to the hits with which this method is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are making assumptions on how the loop over the seeding tree happens.
It's definitely error prone...
We need to discuss this.

@slava77
Copy link
Contributor

slava77 commented Apr 18, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/12448/console

)

#del mixedTripletStepSeedsB.pixelTripletGeneratorFactory.SeedComparitorPSet
Copy link
Contributor

Choose a reason for hiding this comment

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

get rid of comment

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

const DetLayer * thirdLayer = measurementTracker->geometricSearchTracker()->detLayer(thirdHit->det()->geographicalId());
std::vector<const DetLayer *> thirdLayerDetLayer(1,thirdLayer);
std::vector<BaseTrackerRecHit const *> thirdHits(1,(const BaseTrackerRecHit*) thirdHit->hit());
const RecHitsSortedInPhi* thm=new RecHitsSortedInPhi (thirdHits, selectedTrackingRegion->origin(), thirdLayer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a memory leak indeed.
We are making some changes to the seedfinder class to allow for a cleaner selector that doesn't require new statements, is easier to read and a bit more robust.

@a-kapoor
Copy link
Contributor Author

@slava77 @lveldere @matt-komm @makortel Thanks all, suggestions will be implemented in a new pull request. If you have more suggestions in the mean while, you can comment on this thread also.

@slava77
Copy link
Contributor

slava77 commented Apr 19, 2016

Why new pull request?
It may be better if you can update this one so that we don't loose the comments.

@slava77
Copy link
Contributor

slava77 commented Apr 26, 2016

-1
needs a rebase or a replacement

@davidlange6
Copy link
Contributor

This PR looks obsolete - will close.

@lveldere
Copy link
Contributor

Yes, obsolete indeed.
Lukas

On Fri, Apr 29, 2016 at 11:18 AM, David Lange notifications@github.com
wrote:

This PR looks obsolete - will close.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14083 (comment)

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