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

double EM enrichment filter support for HepMCfilter #22980

Merged

Conversation

andreh7
Copy link

@andreh7 andreh7 commented Apr 16, 2018

This pull request adds support for using the double EM enrichment filter code in the HepMCfilter framework as well as using the same filter code still from the 'classic' PythiaFilterGammaGamma.

(this will allow generating double EM enriched samples with Madgraph/aMC@NLO more efficiently by retrying PYTHIA hadronization multiple times per hard event)

As suggested by @alberto-sanchez we also included a standalone test configuration (GeneratorInterface/GenFilters/test/test_doubleEMEnrichingHepMCfilter.py) running on a 35 event .lhe file .

The code builds and runs on CMSSW_10_2_X_2018-04-11-2300 .

@gourangakole has done some testing of the filtered events.

Please let us know if you require changes/additions etc.

ah added 15 commits April 12, 2018 11:46
…nges needed to make this a BaseHepMCFilter instead of a EDFilter
…to GeneratorInterface/Core to avoid circular dependency / missing symbols at linking time
…an unknown enrichment filter to a later segmentation violation
… factored out from it into PythiaHepMCFilterGammaGamma
@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-22980/4363

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22980/4363/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22980/4363/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #22980 was updated. @cmsbuild, @efeyazgan, @perrozzi can you please check and sign again.

@fabiocos
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 20, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27584/console Started: 2018/04/20 14:23

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@fabiocos
Copy link
Contributor

@andreh7 ok, the only way t avpid the root dependdency would be to move the class to use the FourVector from SimpleVector in HepMC. I think it could be fine like this, GenFilters already depends on root.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2492830
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2492653
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 23 files compared)
  • Checked 119 log files, 9 edm output root files, 29 DQM output files

@fabiocos
Copy link
Contributor

@perrozzi although you had already sign and the change was minor, could you please sign it again?

@perrozzi
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

+1

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

5 participants