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

Add variables for PileUp Jet ID BDT in JME Custom-NanoAODs. #28430

Merged
merged 4 commits into from
Dec 9, 2019

Conversation

nurfikri89
Copy link
Contributor

PR description:

This PR adds jet variables needed to train the BDT for PileUp jet ID in the JME Custom-NanoAODs which are not included in the main NanoAODs. The variables are not stored in MiniAODs so they need to be re-calculated again by scheduling pileupJetIdCalculator. Then, the variables need to be stored in ValueMaps and this is done by scheduling PileupJetIDVarProducer plugin which will be added also. The rest is just normal procedure to save variables in NanoAODs. At the moment, these variables are set to be calculated and stored only for AK4PFCHS jets.

This PR needs to be backported to 10_6_X, once merged, for the ultra legacy campaigns.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@nurfikri89 nurfikri89 changed the title From1100pre11 nanojme pujetidvar Add variables for PileUp Jet ID BDT in JME Custom-NanoAODs. Nov 20, 2019
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28430/12827

  • This PR adds an extra 24KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@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-28430/12828

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @nurfikri89 (Nurfikri Norjoharuddeen) for master.

It involves the following packages:

PhysicsTools/NanoAOD

@cmsbuild, @santocch, @fgolf, @peruzzim can you please review it and eventually sign? Thanks.
@gpetruc 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

@smuzaffar smuzaffar modified the milestones: CMSSW_11_0_X, CMSSW_11_1_X Dec 2, 2019
@fabiocos
Copy link
Contributor

fabiocos commented Dec 3, 2019

@peruzzim @fgolf could you please scrutinize this PR? It is fully in your domain

@fabiocos
Copy link
Contributor

fabiocos commented Dec 3, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3750/console Started: 2019/12/03 14:57

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2793840
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2793497
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@peruzzim
Copy link
Contributor

peruzzim commented Dec 5, 2019

+xpog

The code looks ok to me, from a technical point of view. Modifications are confined to the PrepJMECustomNanoAOD(process) customise function, and will not affect standard NANOAOD production.
On the other hand, I think that no relval workflow is currently implemented to test the JME-custom NANOAOD production. I assume it was thoroughly tested by JME to ensure it satisfy their needs.
In any case, I would encourage JME to add this check, to be sure we don't inadvertently break this workflow in the future.

@fabiocos
Copy link
Contributor

fabiocos commented Dec 5, 2019

@nurfikri89 could you please comment about the performed validation?

@santocch
Copy link

santocch commented Dec 8, 2019

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2019

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

fabiocos commented Dec 9, 2019

@nurfikri89 could you please comment about the performed validation?

@nurfikri89
Copy link
Contributor Author

Hi @fabiocos, the only validation for this PR that we had done is to check that the variables we are adding are the same as the ntuples produced by PileUp Jet ID analyzers (like this) and that the values are not different between the CustomNanoAOD and the ntuples. Maybe the JMAR convenors can comment more on this. @camclean @alefisico

Apologies. I have not setup any validation workflow as suggested by @peruzzim. Is there an example which I can based on?

@fabiocos
Copy link
Contributor

fabiocos commented Dec 9, 2019

@nurfikri89 for the PR itself I believe that your reference can be ok, but I agree with @peruzzim that adding a dedicated workflow to produce this non-standard NANOAOD (in the spirit of one of the existing 132X workflows) would be useful.

@peruzzim
Copy link
Contributor

peruzzim commented Dec 9, 2019

If this workflow will be run centrally by PdmV, as I understand will be the case, they will need to define a pilot in the relval system to check it before injection. I think would be the right occasion to have them add the configuration to the relval matrix tests.

@fabiocos
Copy link
Contributor

fabiocos commented Dec 9, 2019

+1

@cmsbuild cmsbuild merged commit 5cff934 into cms-sw:master Dec 9, 2019
cmsbuild added a commit that referenced this pull request Jan 8, 2020
…pujetidvar

Backport of #28430 (Add variables for PileUp Jet ID BDT in JME Custom-NanoAODs)
@nurfikri89 nurfikri89 deleted the from1100pre11_nanojme_pujetidvar branch June 21, 2020 05:12
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