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

Final BDT models based on 10.2 MC samples #13

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

bainbrid
Copy link
Contributor

@bainbrid bainbrid commented Feb 14, 2019

@slava77 @perrotta

These BDT models are used by the LowPtGsfElectronSeedProducer and LowPtGsfElectronIDProducer modules in the low pT electron reconstruction.

They are to be used in conjunction with by this PR: #25936.

They are intended for the 10_2_X release cycle only. (We will release update models for the 10_5_X release cycles and later.)

The performance of the two Seed models is shown below and compared to the default EGamma seeding approach (blue circle).

image

@bainbrid
Copy link
Contributor Author

@mverzett is interested to follow this thread.

@cmsbuild
Copy link
Contributor

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

@cmsbuild, @smuzaffar, @gudrutis, @mrodozov can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

external issue cms-sw/cmsdist#4702

@mrodozov
Copy link
Contributor

assign reconstruction

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

@mrodozov
Copy link
Contributor

@slava77 ping

@slava77
Copy link

slava77 commented Feb 18, 2019

@slava77 ping

@perrotta will probably have a final say on this.

@bainbrid
I'm curious if all 9 new files (about 200MB) are really going to be used in production jobs.
In particular, I only see the 3 xml.gz files used in cms-sw/cmssw#25936.
Please clarify on the pkl files.

@bainbrid
Copy link
Contributor Author

@slava77

Only the three Autumn18 files will be used.

The .xml files define the models used as defined by the library used to generate them (scikit-learn), before their conversion to be compliant with TMVA. They are intended to be kept for reproducibility in the future.

Do you prefer we remove all unnecessary files from the PR?

@slava77
Copy link

slava77 commented Feb 19, 2019 via email

@cmsbuild
Copy link
Contributor

Pull request #13 was updated.

external issue cms-sw/cmsdist#4702

@bainbrid
Copy link
Contributor Author

@slava77
Removed obsolete files and squashed the commit involving the pkl files.

@perrotta
Copy link

@mrodozov @smuzaffar : could you please prepare two cmsdata pull requests out of this one, one for 10_5_X and one for 10_6_X?

Thank you!

@bainbrid
Copy link
Contributor Author

bainbrid commented Feb 21, 2019 via email

@mrodozov
Copy link
Contributor

So 10_2 only it is for now if I got that right ?

@perrotta
Copy link

perrotta commented Feb 22, 2019 via email

@mrodozov mrodozov merged commit f747396 into cms-data:master Feb 22, 2019
mrodozov added a commit to cms-sw/cmsdist that referenced this pull request Feb 22, 2019
@bainbrid
Copy link
Contributor Author

@mrodozov @smuzaffar

After discussing internally, and in the RECO meeting on Friday, we think it would be a good idea to propagate these models to master.

Please could you please prepare a new cmsdata pull request out of this one, to be used in master. Ideally, we’d back port to 10_5_X as well, if everyone agrees?

I will need to create a new corresponding PR for cms-sw to update the configs to point to the new models (which have different names) and set the appropriate working points. I’ll do this later this evening.

@perrotta
Copy link

@mrodozov @smuzaffar could you please prepare the corresponding of #4724 for the master, so that the tests for #26012 can get started?
Thank you!

@smuzaffar
Copy link
Contributor

@gudrutis , can you please take care of this? Just update the cmsdist/data/cmsswdata.txt with newer version

@gudrutis
Copy link

I updated RecoEgamma-ElectronIdentification to V01-01-03 on master and started the tests on #26012 .

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.

7 participants