-
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
MXNet-based implementation of the ParticleNet tagger #28902
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ae7a00b
MXNet-based ParticleNet V00.
hqucms f465b35
Fix NaN.
hqucms cb8bd7b
Add ParticleNet to MiniAOD sequence.
hqucms 7a7c0a7
Add a test cfg.
hqucms 567839a
Change ParticleNet model paths.
hqucms 878def8
Apply code checks.
hqucms 4d8bfcb
Address review comments.
hqucms d1af63d
Simply preprocess params.
hqucms 3c9ce7f
Merge TagInfo producers.
hqucms File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 an AK8 suffix be added?
We didn't do it for the boosted and still manage. So, perhaps we can keep pretending that there is no confusion.
@ferencek did the topic of the parent jet cone confusion show up in some recent (1-2 years) discussions?
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.
@slava77, in the past we had examples of
AK8
andCA15
added in the TagInfo names, even when there wasBoosted
somewhere in the name if more than one cone size was supported for fat jets. If long term ParticleNet tagging coukd be supported for multiple cone sizes, having an extra suffix might be a good idea just to be more explicit and avoid potential confusion down the road.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.
@hqucms
so, it sounds like AK8 here (and for the tags) would help
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.
@slava77
Thinking about it a bit more -- I think it's actually better not to add the AK8 here as the infrastructure is generic and applies to any jet sizes. What we can do (and we are doing now in MiniAOD/NanoAOD) is to use
postfix
to distinguish different jet types (e.g., in applyDeepBtagging_cff.py, we usepostfix=AK8WithDeepInfo
so the tagInfo product is calledpfParticleNetTagInfosAK8WithDeepInfo
). Hardcoding AK8 into the name seems to make it awkward to use it for other jet sizes (actually we do have another use case for AK15 using the same infrastructure).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.
uhm, but pfParticleNetTagInfos by itself is configured to use AK8.
Also, I understood that a major effort done by JME to put PUPPI into the RECO workflow was to then enable running the ML taggers (the ones using PUPPI) in RECO as well.
For that case, this has a specific meaning and not a somewhat magic key for applyDeepBtagging_cff.py to call PhysicsTools/PatAlgos/python/tools/jetTools.py
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.
@slava77 I think
PhysicsTools/PatAlgos/python/tools/jetTools.py
is also called when adding b-taggers in RECO, no?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.
ehm, nevermind this file itself is
a part of PAT.
OK then