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.

Sign up for GitHub

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

Jump to bottom

Fix charged and neutral candidate converters for reco::Jets #35922

Merged
merged 4 commits into from Nov 11, 2021
Merged

Fix charged and neutral candidate converters for reco::Jets #35922

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
082f9db
allow reco::Jets in candidate converters
SWuchterl Oct 29, 2021
2301a29
small code improvements
SWuchterl Nov 4, 2021
b9c58b9
fix max element search
SWuchterl Nov 9, 2021
758843c
code style
SWuchterl Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 14 additions & 12 deletions RecoBTag/FeatureTools/interface/ChargedCandidateConverter.h
View file
Open in desktop
Expand Up @@ -53,20 +53,22 @@ namespace btagbtvdeep {

// subjet related
const auto* patJet = dynamic_cast<const pat::Jet*>(&jet);
if (!patJet) {
throw edm::Exception(edm::errors::InvalidReference) << "Input is not a pat::Jet.";
}

if (patJet->nSubjetCollections() > 0) {
auto subjets = patJet->subjets();
// sort by pt
std::sort(subjets.begin(), subjets.end(), [](const edm::Ptr<pat::Jet>& p1, const edm::Ptr<pat::Jet>& p2) {
return p1->pt() > p2->pt();
});
c_pf_features.drsubjet1 = !subjets.empty() ? reco::deltaR(*c_pf, *subjets.at(0)) : -1;
c_pf_features.drsubjet2 = subjets.size() > 1 ? reco::deltaR(*c_pf, *subjets.at(1)) : -1;
if (patJet) {
if (patJet->nSubjetCollections() > 0) {
auto subjets = patJet->subjets();
// sort by pt
std::sort(subjets.begin(), subjets.end(), [](const edm::Ptr<pat::Jet>& p1, const edm::Ptr<pat::Jet>& p2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for a full sorting here.

Copy link
Contributor Author

@SWuchterl SWuchterl Nov 1, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those pieces of the code were already there, I just inverted the logic of the if statement in the beginning. From the diff it seems different, I know. But of course, I can change it as suggested.
(Also applies for the second comment below)
Just for clarification, do you suggest removing the line entirely because the collection should already be sorted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stil this code is very sub-optimal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SWuchterl I understand you only need the first and second jet by pt, so you don't need to do a full sorting. This could be a straightforward improvement for both the new and original code. I'm not sure you can safely assume that the collection is sorted.

We can also run a profiling on this PR to check if this is a measurable/detectable issue.

return p1->pt() > p2->pt();
});
c_pf_features.drsubjet1 = !subjets.empty() ? reco::deltaR(*c_pf, *subjets.at(0)) : -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use operator[] not at

c_pf_features.drsubjet2 = subjets.size() > 1 ? reco::deltaR(*c_pf, *subjets.at(1)) : -1;
} else {
// AK4 jets don't have subjets
c_pf_features.drsubjet1 = -1;
c_pf_features.drsubjet2 = -1;
}
} else {
// AK4 jets don't have subjets
c_pf_features.drsubjet1 = -1;
c_pf_features.drsubjet2 = -1;
}
Expand Down
24 changes: 13 additions & 11 deletions RecoBTag/FeatureTools/interface/NeutralCandidateConverter.h
View file
Open in desktop
Expand Up @@ -32,18 +32,20 @@ namespace btagbtvdeep {
NeutralCandidateFeatures& n_pf_features) {
const auto* patJet = dynamic_cast<const pat::Jet*>(&jet);

if (!patJet) {
throw edm::Exception(edm::errors::InvalidReference) << "Input is not a pat::Jet.";
}
// Do Subjets
if (patJet->nSubjetCollections() > 0) {
auto subjets = patJet->subjets();
// sort by pt
std::sort(subjets.begin(), subjets.end(), [](const edm::Ptr<pat::Jet>& p1, const edm::Ptr<pat::Jet>& p2) {
return p1->pt() > p2->pt();
});
n_pf_features.drsubjet1 = !subjets.empty() ? reco::deltaR(*n_pf, *subjets.at(0)) : -1;
n_pf_features.drsubjet2 = subjets.size() > 1 ? reco::deltaR(*n_pf, *subjets.at(1)) : -1;
if (patJet) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why all this code duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify the comment about what code exactly is duplicated in this file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code in this file looks identical to the one in the previous one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SWuchterl for clarification, the duplication is between ChargedCandidateConverter.h and NeutralCandidateConverter.h. The new code could be a simple utility function defined once, and called from both files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first file is about Charged candidates and this one is about the Neutral ones. Some elements look similar indeed but they define 2 different inputs used by the neural network.

if (patJet->nSubjetCollections() > 0) {
auto subjets = patJet->subjets();
// sort by pt
std::sort(subjets.begin(), subjets.end(), [](const edm::Ptr<pat::Jet>& p1, const edm::Ptr<pat::Jet>& p2) {
return p1->pt() > p2->pt();
});
n_pf_features.drsubjet1 = !subjets.empty() ? reco::deltaR(*n_pf, *subjets.at(0)) : -1;
n_pf_features.drsubjet2 = subjets.size() > 1 ? reco::deltaR(*n_pf, *subjets.at(1)) : -1;
} else {
n_pf_features.drsubjet1 = -1;
n_pf_features.drsubjet2 = -1;
}
} else {
n_pf_features.drsubjet1 = -1;
n_pf_features.drsubjet2 = -1;
Expand Down