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

New_samples (TTbarLepton_14 and QQH1252T_14) added without updating 'upgradeWorkflowComponents' file #33409

Merged
merged 11 commits into from Apr 28, 2021

Conversation

sharmaaash
Copy link
Contributor

PR description:

Samples "TTbarLepton_14 and QQH1252T_14" added.
No additional changes in 'upgradeWorkflowComponents.py' file

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33409/22046

  • This PR adds an extra 40KB to repository

  • Found files with invalid states:

    • Configuration/Generator/python/QQH1352T_14TeV_TuneCP5_cfi.py:
    • Configuration/Generator/python/QQH1352T_14TeV_TuneCUETP8M1_cfi.py:
    • Configuration/Generator/python/TTbarLepton_14TeV_TuneCUETP8M1_cfi.py:

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @sharmaaash (Ashish Sharma) for master.

It involves the following packages:

Configuration/Generator

@SiewYan, @mkirsano, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @fabiocos this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@SiewYan
Copy link
Contributor

SiewYan commented Apr 12, 2021

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b4fe4d/14190/summary.html
COMMIT: 59487bb
CMSSW: CMSSW_11_3_X_2021-04-12-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33409/14190/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2865526
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2865497
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 37 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: no differences found

@chayanit
Copy link

chayanit commented Apr 20, 2021

Hello @cms-sw/generators-l2 , could we have this PR merged before the final 11_3_0 release? together with #33414 , #33433 and #33386 and also #33407 thanks!

pythia8CP5SettingsBlock,
processParameters = cms.vstring(
'Top:gg2ttbar = on ',
'Top:gg2ttbar = on ',

Choose a reason for hiding this comment

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

what is the reason to have this line twice? is this supposed to be qqbar ?

pythiaHepMCVerbosity = cms.untracked.bool(False),
# put here the cross section of your process (in pb)
crossSection = cms.untracked.double(0.388),
maxEventsToPrint = cms.untracked.int32(3),

Choose a reason for hiding this comment

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

Suggest to set to 0 for relval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean cross section or which parameter to set to zero?

@agrohsje
Copy link

agrohsje commented Apr 20, 2021

Please see my comments on the fragments. About names. Why is it called TTbarLepton ? Instead of QQH1252T I would propose QQToHToTauTau_mh125 or so. The general ending could be TuneCP5_14TeV_pythia8, just to make it easier to understand what is encoded in the fragment.

@cmsbuild
Copy link
Contributor

Pull request #33409 was updated. @SiewYan, @mkirsano, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please check and sign again.

@agrohsje
Copy link

@sharmaaash now the top is called higgstotautau. I think you were misreading my comment. :-)
What about:
QQToHToTauTau_mh125_TuneCP5_14TeV_pythia8_cfi.py
TTbarToDilepton_mt172p5_TuneCP5_14TeV_pythia8_cfi.py
Fragments are ok.

@sharmaaash
Copy link
Contributor Author

QQToHToTauTau_mh125_TuneCP5_14TeV_pythia8_cfi.py

I made changes.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33409/22360

  • This PR adds an extra 28KB to repository

  • Found files with invalid states:

    • Configuration/Generator/python/TTbarLepton_14TeV_TuneCP5_cfi.py:
    • Configuration/Generator/python/QQH1252T_14TeV_TuneCP5_cfi.py:
    • Configuration/Generator/python/QQH1352T_14TeV_TuneCP5_cfi.py:
    • Configuration/Generator/python/QQH1352T_14TeV_TuneCUETP8M1_cfi.py:
    • Configuration/Generator/python/TTbarLepton_14TeV_TuneCUETP8M1_cfi.py:
    • Configuration/Generator/python/QQToHToTauTau_mh125_TuneCP5_14TeV_pythia8_cfi.py:

@cmsbuild
Copy link
Contributor

Pull request #33409 was updated. @SiewYan, @mkirsano, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please check and sign again.

@agrohsje
Copy link

+generators

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b4fe4d/14684/summary.html
COMMIT: 66a3760
CMSSW: CMSSW_12_0_X_2021-04-28-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33409/14684/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2877605
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2877581
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Apr 28, 2021

+1

@cmsbuild cmsbuild merged commit 399dd9e into cms-sw:master Apr 28, 2021
@@ -0,0 +1,29 @@
import FWCore.ParameterSet.Config as cms
from Configuration.Generator.Pythia8CommonSettings_cfi import *
from Configuration.Generator.Pythia8CP5Settings_cfi import *

Choose a reason for hiding this comment

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

same here
we noticed that this import has a wrong path and failed in production. It should be fixed soon.
from Configuration.Generator.MCTunes2017.PythiaCP5Settings_cfi import *

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do I need to create another PR with the following changes with the replacement ---> from Configuration.Generator.MCTunes2017.PythiaCP5Settings_cfi import *
?

Copy link
Contributor

Choose a reason for hiding this comment

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

So do I need to create another PR with the following changes with the replacement ---> from Configuration.Generator.MCTunes2017.PythiaCP5Settings_cfi import *
?

Yes, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made following changes and recreate PR:
#33700

@agrohsje
Copy link

@chayanit You are right. I overlooked this when giving my ok. @qliphy @silviodonato is there a way to include technical checks for python scripts? Or do we always have to ask the person who makes the PR if the script was tested. For more complicated once, like H7 it would be even more likely that such mistakes get not spot when checking by eye. Do we need to rely on local testing or are there alternatives?

@silviodonato
Copy link
Contributor

@agrohsje if you can run the fragment using runTheMatrix.py, we can test the fragment in the PR tests. Otherwise we have to rely on the author or GEN group private test.
All PRs are supposed to be tested by the authors. The tests performed should be reported in the section "PR validation:" in the PR description.

@agrohsje
Copy link

Hi @silviodonato. Ok. So if not mentioned we will double-check. Thanks!

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