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

LowPtElectrons: ID models for 10_6_X #17

Conversation

bainbrid
Copy link
Contributor

@bainbrid bainbrid commented Dec 6, 2020

This PR contains models that are required by PR cms-sw/cmssw#32372 (which is a back port of cms-sw/cmssw#31220).

There are two weights files:

RunII_Autumn18_LowPtElectrons_mva_id.xml.gz

  • this is the model used by RECO throughout the 10_6_X release cycle

LowPtElectrons_ID_2020Sept15.root

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2020

A new Pull Request was created by @bainbrid for branch master.

@perrotta, @smuzaffar, @mrodozov, @cmsbuild, @slava77, @jpata can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @lgray, @sobhatta, @afiqaize, @varuns23, @ram1123 this is something you requested to watch as well.
cms-bot commands are listed here

@slava77
Copy link

slava77 commented Dec 6, 2020

I'm confused, what is the difference between this PR and #15?

Both are a single commit and both add a file LowPtElectrons/LowPtElectrons_ID_2020Sept15.root
... and #15 is already merged

@bainbrid
Copy link
Contributor Author

bainbrid commented Dec 7, 2020

In response to: #17 (comment):

In short, this commit changed the RunII_Autumn18_LowPtElectrons_mva_id.xml.gz file. Here's the history for the XML file. This PR reverts that change.

It's difficult to know how to navigate GitHub to find the commit or tag used to build a particular release. I've tried to navigate the history as best I can, details below. I think they are correct.


The two PRs #15 and #17 are branched from a different base, and:

  • they contain different versions of RunII_Autumn18_LowPtElectrons_mva_id.xml.gz
  • while LowPtElectrons_ID_2020Sept15.root is identical
  1. PR LowPtElectrons: latest BDT model for ID (ROOT file format) #15 is based on the branch CMSBParking:from-CMSSW_10_2_15_2020Sept15
> git log --oneline RunII_Autumn18_LowPtElectrons_mva_id.xml.gz
2aa9f04 Extra variables for LowPtElectrons Id added. Depth10, no regression
c79bcd5 added Autumn18 trainings
> git diff 2aa9f04^ 2aa9f04
diff --git a/LowPtElectrons/RunII_Autumn18_LowPtElectrons_mva_id.xml.gz b/LowPtElectrons/RunII_Autumn18_LowPtElectrons_mva_id.xml.gz
index 5c15cb1..29ce4ac 100644

Here is the diff on GitHub: CMSBParking@2aa9f04#diff-72d9ce87b279f68129c5861a9448a971a70c5b38c07373005980eadf0a990d8b.

At the head of this branch, the size of RunII_Autumn18_LowPtElectrons_mva_id.xml.gz is 8519842 bytes.

  1. PT LowPtElectrons: ID models for 10_6_X #17 (this PR) is based on the branch CMSBParking:LowPtElectrons_ID_2020Sept15_for106X
> git log --oneline RunII_Autumn18_LowPtElectrons_mva_id.xml.gz gives:
c79bcd5 added Autumn18 trainings

At the head of this branch, the size of RunII_Autumn18_LowPtElectrons_mva_id.xml.gz is 7599273 bytes.

@slava77
Copy link

slava77 commented Dec 7, 2020

I still could not follow the description given what's showing up in the web interface: only LowPtElectrons/LowPtElectrons_ID_2020Sept15.root is changed in this PR, while the description is about RunII_Autumn18_LowPtElectrons_mva_id.xml.gz

@bainbrid
Copy link
Contributor Author

bainbrid commented Dec 7, 2020

This is the commit history for the branch for this PR (#17):
https://github.com/CMSBParking/RecoEgamma-ElectronIdentification/commits/from-CMSSW_10_2_15_2020Sept15

  • The penultimate commit is a merge from PR Final BDT models based on 10.2 MC samples  #13 (dated Feb 2019), which contains the xml.gz file used throughout the 10_6_X cycle. Importantly, this is the file we need to maintain backward compatibility for the RECO sequence in 10_6_X.
  • The last commit is the one I've just made, which adds the 2020Sept15 model for reminiAOD.

This is the commit history for the branch for PR #15:
https://github.com/CMSBParking/RecoEgamma-ElectronIdentification/commits/from-CMSSW_10_2_15_2020Sept15

This is the commit history for the master of cms-data:
https://github.com/cms-data/RecoEgamma-ElectronIdentification/commits/master

  • In short, we want to revert the change in commit 2aa9f04

@slava77
Copy link

slava77 commented Dec 7, 2020

This is the commit history for the branch for this PR (#17):

all of this apparently means that we

  • need a PR to a branch corresponding to 10_6_X, which does not exist
  • or we need to explicitly revert the update in the xml file in the master (remake this PR on top of the master)

@slava77
Copy link

slava77 commented Dec 7, 2020

@smuzaffar @mrodozov
we will need a separate branch for this external for the update to 10_6_X.

I see that the current version in 10_6_X is V01-01-03,
perhaps V01-01-03_X would work, but feel free to suggest another branch name (and go ahead to make it, please).

@smuzaffar smuzaffar changed the base branch from master to V01-01-XX December 7, 2020 18:25
@smuzaffar
Copy link
Contributor

smuzaffar commented Dec 7, 2020

@slava77 , I created a new branch V01-01-XX (based on tag V01-01-03) and updated this PR to be based on it.
If this PR is good to go then we can merge and create a tag V01-01-04.

@slava77
Copy link

slava77 commented Dec 7, 2020

@slava77 , I created a new branch V01-01-XX (based on tag V01-01-03) and updated this PR to be based on it.
If this PR is good to go then we can merge and create a tag V01-01-04.

Thank you.
Let me see what comes out from tests with cms-sw/cmssw#32372

@slava77
Copy link

slava77 commented Dec 8, 2020

+1

@slava77 , I created a new branch V01-01-XX (based on tag V01-01-03) and updated this PR to be based on it.
If this PR is good to go then we can merge and create a tag V01-01-04.

Thank you.
Let me see what comes out from tests with cms-sw/cmssw#32372

I think that the last set of tests was good and we can go ahead with merging this PR.

@mrodozov
Copy link
Contributor

mrodozov commented Dec 8, 2020

+externals

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2020

This pull request is fully signed and it will be integrated in one of the next V01-01-XX 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)

@mrodozov mrodozov merged commit 086aa9d into cms-data:V01-01-XX Dec 8, 2020
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2020

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d9877/11433/summary.html
CMSSW: CMSSW_11_3_X_2020-12-08-1100
SCRAM_ARCH: slc7_amd64_gcc900

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2020

Comparison results are now available
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d9877/11433/summary.html
CMSSW: CMSSW_11_3_X_2020-12-08-1100
SCRAM_ARCH: slc7_amd64_gcc900

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2747014
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2746991
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 153 log files, 37 edm output root files, 36 DQM output files

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.

5 participants