-
Notifications
You must be signed in to change notification settings - Fork 218
feat: Introduce TrackToTruthJetAlgorithm #4637
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
base: main
Are you sure you want to change the base?
Conversation
…o trackToTruthAlg
…o trackToTruthAlg
Co-authored-by: Paul Gessinger <[email protected]>
Co-authored-by: Paul Gessinger <[email protected]>
Rename track jets
…o trackToTruthAlg
…o trackToTruthAlg
|
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 we call this TrackJetAssocationAlgorithm
? Ultimately it shouldn't matter what type of jet this is, right?
Overall, I'm still a bit unsure about the terminology. In my mind we have two types of jets:
- A truth jet is a jet clustered from truth particles. It can then get in addition a set of associated track, but it's still a truth jet
- A track jet is a jet clustered from tracks. Here associating additional track doesn't make whole lot of sense.
Both types of jets can carry a label indicating the jet flavor.
void addTrackToTruthJet(Context& ctx) { | ||
auto mex = ctx.get("examples"); | ||
|
||
ACTS_PYTHON_DECLARE_ALGORITHM(TrackToTruthJetAlgorithm, mex, |
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.
Can you do this in the existing addTruhJet
function?
|
||
namespace ActsPython { | ||
void addTruthJet(Context& /*ctx*/) {} | ||
void addTrackToTruthJet(Context& /*ctx*/) {} |
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.
I think this can be done in the same function if we're not going to build either of them separately and not the other one.
if args.jet_clustering: | ||
args.reco = False # disable track reco if jet clustering is requested | ||
args.ttbar = True # force ttbar for jet clustering |
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.
What's the idea behind this? In principle, the algorithm should work either way right?
) | ||
|
||
if args.jet_clustering: | ||
addSeeding( |
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.
Why do we need a separate track reco in the jet case? Can't this just use the track reco that is configured otherwise?
fastjet::ClusterSequence m_clusterSeq{}; | ||
}; | ||
|
||
class TruthJetBuilder { |
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.
The naming is a bit confusing to me: In the end we have a class that represents a jet, and that jet has constituents. At the most basic level, it shouldn't matter a lot what the constituents are. FastJet's own data model uses the four momentum of whatever object that was passed in.
The main thing I see the need for is that we want to (optionally) support associating additional objects like tracks to a jet, and we want to ultimately store the jet label on the object.
TruthJetBuilder
does not seem to build anything, it just holds the four momentumJetProperties
is a separate class, which I don't fully understand
Maybe we could use inheritance for this (but I'm not sure jet).
- We could have a
Jet
base class that stores the four momentum associated with the jet. - Then we could have a
TruthJet
class that carries a vector of particles as constituents. Since this is a separate plugin, we'd have to stick to using indices, or we make this plugin depend on Fatras so we can pull in either the barcode or the particle type itself. In addition this class would carry a vector of indices for the associated tracks, to avoid having to template on the track container type. (This could be solved in the future if we add a type-erased track proxy type) - A separate
TrackJet
class could instead hold indices to tracks as the constituents.
I guess in the first iteration, since the constituents would always just be indices, the member and methods could also live in the Jet
base class, until the types are sorted out down the line.
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.
My two cents: Having a base Jet
class makes a lot of sense. Both truth-based and track-based forms quack like a jet and walk like a jet - they were just built in different ways. They both (I assume) would still be able to associate with tracks in their cone, and true matching particles. Subclassing I guess is just a convenience to avoid confusion. But keeping as much of the properties and methods in a base will make life a lot easier later when comparing e.g. ceiling performance of jet reconstruction vs. real performance. Then it will be trivial (change the name of the collection).
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.
I still not fully understand.
A jet is a well-defined object that comes from the clustering of other objects, whatever they are (truth particles, tracks, calo clusters, ... ) under certain rules.
In my opinion already having TruthJets and TrackJets is even too much as they really are the same thing just constructed with different objects. Also, a reco jet can still have a truth label associated to it, which is the regular way jets are identified for tagging purposes.
In my view we only would need a Jet class, which holds vectors with associated objects: tracks, particles or calo clusters (if available). To my view this is a similar idea of what ATLAS uses (https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Event/xAOD/xAODJet/xAODJet/versions/Jet_v1.h)
If we want to make more derived classes, keep in mind that truthJets can have both particles and tracks associated and so on, so a truth jet should have a method to associate tracks to it.
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.
which holds vectors with associated objects: tracks, particles or calo clusters (if available)
that's precisely what I think we could use the subclasses for. Otherwise you'd have these objects on all jets, or the constituents are not strongly typed.
class Jet {
Vector4 m_fourMomentum;
};
class TruthJet : public Jet {
std::vector<SimParticle> m_constituents;
std::vector<TrackProxy> m_associatedTracks; // difficult due to lack of type-erasure
};
class TrackJet : public Jet {
std::vector<TrackProxy> m_constituents; // difficult due to lack of type-erasure
};
+ various getters setters etc.
whereas with a single class it would have to be
class Jet {
Vector4 m_fourMomentum;
std::vector<unsigned int> m_constituents; // undefined what these are and which collection to index into
std::vector<unsigned int> m_tracks; // redundant if `m_constituents` are already tracks
std::vector<unsigned int> m_caloClusters;
};
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.
personally I would not couple reconstruction and truth EDMs. the redundancy is quite small
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.
I think my main point is that I'd prefer having a strongly typed TruthJet
where it's clear that the constituents are truth particles, and ideally are linked as strong types as well.
class TruthJetBuilder { | ||
public: | ||
explicit TruthJetBuilder(const Acts::Vector4& fm) { m_fourMomentum = fm; } | ||
const Acts::Vector4 getTruthJetFourMomentum() const { return m_fourMomentum; } |
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.
Generally, a method like this could just be called fourMomentum
in my opinion.
Acts::Vector4 m_fourMomentum{0., 0., 0., 0.}; | ||
}; | ||
|
||
class JetProperties { |
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.
Why do we need this class?
The following methods are all valid for a jet object.
Why do we need a jet and a jet property class?
I think the following members could (and maybe should) be associated to a jet object, in my opinion.
} | ||
/// @brief Get the jet constituents | ||
/// @return the indices of the constituent tracks | ||
const std::vector<int>& getConstituents() const { return m_constituents; } |
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.
If this class uses the truth jet builder, why the constitues are commented as "constituent tracks" ?
They are particles, I would suppose.
fastjet::ClusterSequence m_clusterSeq{}; | ||
}; | ||
|
||
class TruthJetBuilder { |
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.
I still not fully understand.
A jet is a well-defined object that comes from the clustering of other objects, whatever they are (truth particles, tracks, calo clusters, ... ) under certain rules.
In my opinion already having TruthJets and TrackJets is even too much as they really are the same thing just constructed with different objects. Also, a reco jet can still have a truth label associated to it, which is the regular way jets are identified for tagging purposes.
In my view we only would need a Jet class, which holds vectors with associated objects: tracks, particles or calo clusters (if available). To my view this is a similar idea of what ATLAS uses (https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Event/xAOD/xAODJet/xAODJet/versions/Jet_v1.h)
If we want to make more derived classes, keep in mind that truthJets can have both particles and tracks associated and so on, so a truth jet should have a method to associate tracks to it.
In this PR, an algorithm that matches Kalman tracks to jets, which are clustered from truth particles using FastJet (See PR:4267), is introduced. In addition to that, a TruthJetBuilder class, that does jet clustering using truth particles, and a JetProperties class to read track and truth jet properties are introduced Additional WIP: Overlap removal, jet labeling, use of HepMC3 event.