Navigation Menu

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

B parking Skimming code update #25984

Merged
merged 2 commits into from Feb 25, 2019

Conversation

gkaratha
Copy link
Contributor

Dear all,

Since there was a conflicts due to merging of the BParking skimming and our need to move to low pT e both in the analysis and for skimming, I opened a new PR without conflicts. Proposed by @kfjack . If everyone agrees we can close the previous PR and continue the discussion here? Adding @bainbrid and @mverzett . Thanks.

Best regards,
George Karathanasis

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25984/8492

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @gkaratha for master.

It involves the following packages:

Configuration/Skimming

@cmsbuild, @zhenhu, @prebello, @pgunnell can you please review it and eventually sign? Thanks.
@Martin-Grunewald 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

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@gkaratha
Copy link
Contributor Author

Dear all,

In order to be in syncc with the latest developments in low pT electrons, I changed how the BDT value is read by the code (ie first go from electron -> gsfTrack -> read value). The BDT values are the same and the change is technical only. The WP gives the same performance (Rate 5.35%) when running on the same dataset. Thanks.

Best,
George

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25984/8511

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

Pull request #25984 was updated. @cmsbuild, @zhenhu, @prebello, @pgunnell can you please check and sign again.

@cmsbuild
Copy link
Contributor

-1

Tested at: 79cb215

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

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

The relvals timed out after 4 hours.

@cmsbuild
Copy link
Contributor

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

@gkaratha
Copy link
Contributor Author

Dear all,

Checking the logs I see that the reason of failure is due to this: Module: MixingModule:mix (crashed). Naively i would think that skimming does not actually affects mixing module. The reason is that probably the skimming will run after generation (and mixing) is finished. The other argument is that essentially the code is the same with previous time with the difference of using low pT e as input. Can we try to test without the skim or with the previous skim? I will also try to reproduce it locally. Many thanks.

Best,
george

@fabiocos
Copy link
Contributor

the crash looks unrelated to this PR

@fabiocos
Copy link
Contributor

please test workflow 136.898

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 24, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33253/console Started: 2019/02/24 22:20

@cmsbuild
Copy link
Contributor

-1

Tested at: 79cb215

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

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

The relvals timed out after 4 hours.

@cmsbuild
Copy link
Contributor

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

@fabiocos
Copy link
Contributor

+1

the test failure is unrelated to this PR (it originally succeeded), the test specific to the B parking skim passes smoothly

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit ad2dd70 into cms-sw:master Feb 25, 2019
@bainbrid
Copy link
Contributor

@fabiocos
Please could you clarify if this will enter 10_5_X, or just 10_6_X?

@fabiocos
Copy link
Contributor

@bainbrid this is for 10_6_X, now, and I understand you are interested into a 10_2_X backport. Could you please clarify why 10_5_X would be eventually interesting for this PR?

@bainbrid
Copy link
Contributor

It's not necessary, we can just go with 10_6_X.

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