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

Hit pair implementation #13051

Merged
merged 14 commits into from Jan 28, 2016
Merged

Conversation

a-kapoor
Copy link
Contributor

Changes expected after the pull request are explained in this presentation:
FullSimVsFastSim_IterationComparison_Debugging_Tracking_POG_Meeting_18Jan16.pdf
Hit pair check for seed implemented.

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

FastSimulation/Tracking
RecoTracker/TkHitPairs

@civanch, @lveldere, @cvuosalo, @ssekmen, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mschrode, @matt-komm, @gpetruc, @istaslis, @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

@VinInn
Copy link
Contributor

VinInn commented Jan 25, 2016

from a visual inspection the reco part looks purely technical.
Should not produce neither regression nor timing issues.

@@ -43,7 +43,7 @@ class TrajectorySeedProducer:
const MagneticFieldMap* magneticFieldMap;
const TrackerGeometry* trackerGeometry;
const TrackerTopology* trackerTopology;

//const RecHitsSortedInPhi * ih;
Copy link
Contributor

Choose a reason for hiding this comment

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

here and in other modified files, please cleanup the commented out code, unless there is a good reason to keep 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 !!

@slava77
Copy link
Contributor

slava77 commented Jan 26, 2016

@Ansh0712 please clarify when should we expect updates to this PR.
Thank you.

@a-kapoor
Copy link
Contributor Author

In a couple of hours.

-----Original Message-----
From: "Slava Krutelyov" notifications@github.com
Sent: ‎26/‎01/‎2016 20:35
To: "cms-sw/cmssw" cmssw@noreply.github.com
Cc: "Anshul Kapoor" anshul.kapoor@students.iiserpune.ac.in
Subject: Re: [cmssw] Hit pair implementation (#13051)

@Ansh0712 please clarify when should we expect updates to this PR.
Thank you.

Reply to this email directly or view it on GitHub.

@cmsbuild
Copy link
Contributor

Pull request #13051 was updated. @civanch, @lveldere, @cvuosalo, @ssekmen, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #13051 was updated. @civanch, @lveldere, @cvuosalo, @ssekmen, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

@@ -167,5 +182,8 @@ HitDoublets HitPairGeneratorFromLayerPair::doublets( const TrackingRegion& regio
}
LogDebug("HitPairGeneratorFromLayerPair")<<" total number of pairs provided back: "<<result.size();
result.shrink_to_fit();
return result;
//return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out. Ya it is not needed.

@cmsbuild
Copy link
Contributor

Pull request #13051 was updated. @civanch, @lveldere, @cvuosalo, @ssekmen, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

@a-kapoor
Copy link
Contributor Author

please test

Few lines got deleted erroneously.

@slava77
Copy link
Contributor

slava77 commented Jan 27, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@lveldere
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Jan 28, 2016

+1

for #13051 ca049df

  • code changes are in line with the description; expected to change only behavior in fastsim
  • jenkins tests pass and comparisons with baseline show differences only in fastsim workflows (posted earlier for a previous commit .. there were no functional changes since then a-kapoor/cmssw@d960f57...ca049df
    )

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Jan 28, 2016
@cmsbuild cmsbuild merged commit 9531693 into cms-sw:CMSSW_8_0_X Jan 28, 2016
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

6 participants