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

adding onnx models for DeepVertex and DeeJet+DeepVertex combination #37

Merged
merged 5 commits into from Jan 12, 2021

Conversation

leonardogiannini
Copy link
Contributor

As in the title: I'm adding onnx models for DeepJet and DeepVertex and removing the .pb models for these taggers

A PR to cmssw will integrate this update

@emilbols

@cmsbuild
Copy link
Contributor

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

@perrotta, @smuzaffar, @mrodozov, @cmsbuild, @slava77, @jpata can you please review it and eventually sign? Thanks.
@emilbols, @smoortga, @riga, @JyothsnaKomaragiri, @ferencek, @andrzejnovak this is something you requested to watch as well.
cms-bot commands are listed here

@mrodozov
Copy link
Contributor

Hi, do you have any cmssw PR to test this with ?
I see one file (entirely?) removed. If you do let me know I'll start the test with it

@leonardogiannini
Copy link
Contributor Author

this is the PR cms-sw/cmssw#31988

Since the new taggers are not in the default workflow, you need to edit manually the expanded cfg to test.

removing the pb file is ok (it's outdated and similarly not in the default workflow)

Even RecoBTag/TensorFlow is all outdated at this point @emilbols ?

@emilbols
Copy link
Contributor

@leonardogiannini In principle we did keep the TF models in this repository for the other taggers, but i am fine with removing the TF version for DeepVertex, after all we are just updating the model format here.

@emilbols
Copy link
Contributor

@mrodozov @jpata @slava77 Can we have this one merged? We need it to run the corresponding merged CMSSW PR cms-sw/cmssw#31988 .

@mrodozov
Copy link
Contributor

please test

@mrodozov
Copy link
Contributor

if something in cmssw was using this (unless not conditionally) I don't see how cms-sw/cmssw#31988 didn't fail.

@leonardogiannini
Copy link
Contributor Author

leonardogiannini commented Jan 12, 2021

if something in cmssw was using this (unless not conditionally) I don't see how cms-sw/cmssw#31988 didn't fail.

The file removed was called by modules updated by the PR, which now use the updated files. It was also called conditionally, so unless one wants to use exactly the release with the older version and switch on the specific module there is no failure.

@mrodozov
Copy link
Contributor

got it. if the test passes im merging it

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56e075/12243/summary.html
COMMIT: f2752ed
CMSSW: CMSSW_11_3_X_2021-01-12-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2716967
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2716938
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 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.

None yet

4 participants