-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
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. |
Hi, do you have any cmssw PR to test this with ? |
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 ? |
@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. |
@mrodozov @jpata @slava77 Can we have this one merged? We need it to run the corresponding merged CMSSW PR cms-sw/cmssw#31988 . |
please test |
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. |
got it. if the test passes im merging it |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56e075/12243/summary.html Comparison SummarySummary:
|
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