-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Updating SONIC ParticleNet Producer and Config Files #43138
Updating SONIC ParticleNet Producer and Config Files #43138
Conversation
…ersions of particlenet and higgs interaction network
As promised, the accompanying model file PR is here: cms-data/RecoBTag-Combined#53 |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43138/37417
|
A new Pull Request was created by @wpmccormack (Patrick McCormack) for master. It involves the following packages:
@jfernan2, @cmsbuild, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
enable profiling |
please test |
@@ -86,6 +93,8 @@ ParticleNetSonicJetTagsProducer::ParticleNetSonicJetTagsProducer(const edm::Para | |||
for (const auto &flav_name : flav_names_) { | |||
produces<JetTagCollection>(flav_name); | |||
} | |||
|
|||
emptyJets_.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not be necessary; vectors are initialized to empty by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
|
||
emptyJets_.clear(); | ||
|
||
if (!countedInputs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason this can't be done in the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kevin, do you mean moving this to the ParticleNetConstructor function here
void ParticleNetConstructor(const edm::ParameterSet &Config_, |
I need the emptyJets_ vector to be cleared for every event, which is why I added it here, instead of just the call at line 97, which you said was not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was about initializing the constants that happens in this if block. (Indeed the clear() call should happen in acquire().)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh I see what you mean. Yes, moved that block into the constructor
@@ -46,6 +46,7 @@ | |||
) | |||
) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete unnecessary change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae5ab4/35496/summary.html Comparison SummarySummary:
|
test parameters: |
please test |
-1 Failed Tests: RelVals RelVals-INPUT RelValsValueError: Undefined workflows: 11824.9001, 24834.9001
Expand to see more relval errors ...RelVals-INPUT
Expand to see more relval errors ... |
test parameters: |
@wpmccormack need to update the paths here |
gahhh, I forgot to push the changes here that sync with recent changes to the structure of cms-data/RecoBTag-Combined#53. Updated now |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43138/37471
|
Pull request #43138 was updated. @mandrenguyen, @cmsbuild, @jfernan2 can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae5ab4/35540/summary.html
Comparison SummarySummary:
|
+1 |
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. @antoniovilela, @rappoccio, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
@jfernan2 beyond igprof being broken, this PR only directly impacts the special workflows ending in .9001, which aren't picked up by the profiling option anyway. |
+1 |
PR description:
Latest release of CMSSW (CMSSW_13) has new versions of particleNet and a new higgs interaction network, which can all be accelerated by current SONIC infrastructure. This existing infrastructure was introduced in previously merged PR: #37964. The new models have different input features, requiring a more robust approach to determining the shapes of tensors to be sent to the model hosting servers. I introduce a scheme to read the preprocessing json files for the models to determine how many of the features are particle flow candidates (pf), secondary vertices (sv), or the newly introduced lost tracks (lt).
Tagging @kpedro88 @yongbinfeng @violatingcp
PR validation:
Outputs of models have been checked locally to match with non-SONIC version of models. I also followed the pre-PR instructions here: https://cms-sw.github.io/PRWorkflow.html. Some tests failed due to xrootd issues, which are not related to this PR as far as I can tell.
There will be dependency on a PR in the RecoBTag-Combined repository. Once I make that PR, I will comment on this one