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.

Sign up for GitHub

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

Jump to bottom

V0branch - modifications to V0Fitter (V0Producer) for PU #5951

Merged
merged 8 commits into from Nov 7, 2014
Merged

V0branch - modifications to V0Fitter (V0Producer) for PU #5951

merged 8 commits into from Nov 7, 2014

Conversation

fojensen
Copy link
Contributor

  1. added new cuts and modified old default values to purify the V0 collection in presence of PU (optimized for PU25). for slides from a tracking POG meeting see:
    https://indico.cern.ch/event/337839/
    https://indico.cern.ch/event/337839/contribution/3/material/slides/0.pdf
  2. deleted code that was no longer relevant to the module
  3. rearranged code to apply selection cuts sooner to help speed the execution of the module

@fojensen fojensen closed this Oct 22, 2014
@fojensen fojensen reopened this Oct 22, 2014
@cmsbuild cmsbuild added this to the Next CMSSW_7_3_X milestone Oct 22, 2014
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fojensen (Frank Jensen) for CMSSW_7_3_X.

V0branch

It involves the following packages:

RecoVertex/V0Producer

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @appeltel, @cerati 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@slava77
Copy link
Contributor

slava77 commented Oct 23, 2014

@fojensen
Frank, please provide a description for this PR and maybe a more descriptive subject line as well.

https://twiki.cern.ch/twiki/bin/viewauth/CMS/RecoIntegration#For_reco_contact
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/CodingAndStyleRules.pdf

Thank you

slava77
slava77 reviewed Oct 23, 2014
View reviewed changes
RecoVertex/V0Producer/src/V0Fitter.cc
const double piMassSquared = piMass*piMass;
const double protonMass = 0.938272046;
const double protonMassSquared = protonMass*protonMass;
const double KShortMass = 0.497614;
Copy link
Contributor

Choose a reason for hiding this comment

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

CMS code style rules ask for variable and method names to start from a lower case; the upper case first letter is reserved for class and type names.

Copy link
Contributor

Choose a reason for hiding this comment

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

this applies to many changed places in this PR

@slava77
Copy link
Contributor

slava77 commented Oct 23, 2014

To summarize the code review:

  • The showstopper is getByLabel (with a missing consumes call).
  • Removal of commented out code is highly desirable.
  • the rest would be very nice to follow up

@cmsbuild
Copy link
Contributor

+1
Tested at: d3a7c70
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5951/294/summary.html

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2014

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

@slava77
Copy link
Contributor

slava77 commented Nov 7, 2014

+1

for #5951 52d5d15
tested in CMSSW_7_3_X_2014-11-03-0200 (test area sign446)

The updated code runs even faster than the old code (perf ticks move from 1.5 to 1.1).

The purity seems to be confirmed:

  • dijet QCD 3 to 3.5 TeV

all_sign446vsorig_qcd13tevpt3ts3t5wf13p0c_recovertexcompositecandidates_generalv0candidates_kshort_reco_obj_mass

  • Photon PD in 2012D
    all_sign446vsorig_runphoton2012bwf4p53c_recovertexcompositecandidates_generalv0candidates_kshort_rereco_obj_mass
  • ttbar with 25ns PU35
    the decrease in efficiency is fairly small (the stats are small though).
    all_sign446vsorig_ttbarpuwf25202p0c_recovertexcompositecandidates_generalv0candidates_kshort_reco_obj_mass

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2014

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

cmsbuild added a commit that referenced this pull request Nov 7, 2014
@cmsbuild
Merge pull request #5951 from fojensen/v0branch
f938172 
V0branch - modifications to V0Fitter (V0Producer) for PU
@cmsbuild cmsbuild merged commit f938172 into cms-sw:CMSSW_7_3_X Nov 7, 2014
@davidlange6
Copy link
Contributor

all - is this PR possibly related to

%MSG
Begin processing the 8th record. Run 1, Event 8, LumiSection 1 at 08-Nov-2014 10:54:21.470 CET
%MSG-w TrajectoryStateClosestToPoint_PerigeeConversions: SecondaryVertexProducer:ghostTrackVertexTagInfos 08-Nov-2014 10:54:22 CET Run: 1 Event: 8
Caught exception An exception of category 'PerigeeConversions' occurred.
Exception Message:
Track with pt=0
.

https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc481/CMSSW_7_3_X_2014-11-08-0200/pyRelValMatrixLogs/run/16.0_SingleElectronPt1000+SingleElectronPt1000INPUT+DIGI+RECO+HARVEST/step3_SingleElectronPt1000+SingleElectronPt1000INPUT+DIGI+RECO+HARVEST.log

@StoyanStoynev
Copy link
Contributor

I don't think so. This PR entered CMSSW_7_3_X_2014-11-08-0200
and my local tests of IB CMSSW_7_3_X_2014-11-06-0200 show the same warning as above:
cmsdev03:/build/stoyan/tests/IB/CMSSW_7_3_X_2014-11-06-0200/src/test/exteneded/16.0_SingleElectronPt1000+SingleElectronPt1000INPUT+DIGI+RECO+HARVEST/step3_SingleElectronPt1000+SingleElectronPt1000INPUT+DIGI+RECO+HARVEST.log

@davidlange6
Copy link
Contributor

ah - yes - ok - so its bad to have someone throwing (but at least catching) for pt=0 - as this happens, but does not lead to a crash. Thanks for the clarification.

On Nov 10, 2014, at 10:48 AM, StoyanStoynev notifications@github.com
wrote:

I don't think so. This PR entered CMSSW_7_3_X_2014-11-08-0200
and my local tests of IB CMSSW_7_3_X_2014-11-06-0200 show the same warning as above:
cmsdev03:/build/stoyan/tests/IB/CMSSW_7_3_X_2014-11-06-0200/src/test/exteneded/16.0_SingleElectronPt1000+SingleElectronPt1000INPUT+DIGI+RECO+HARVEST/step3_SingleElectronPt1000+SingleElectronPt1000INPUT+DIGI+RECO+HARVEST.log

—
Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
comparison-available fully-signed reconstruction-approved tests-approved
Projects
None yet
Milestone
Next CMSSW_7_3_X
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fojensen @cmsbuild @slava77 @Dr15Jones @StoyanStoynev @davidlange6