LorenzVectorBase interfaces#223
Conversation
Add LorentzVectorBase package and define ::FourMomentum (EEJet and PseudoJet) as PxPyPzE coordinate systems. Update getter functions to call the delegated function from LorentzVectorBase. For some functions where edge case behaviour differs from previous behaviour the output is modified to match what happened before: - phi should be in the range [0, 2pi) - rapidity and pseudorapidity should have a maximum cut off for stability Small documentation improvements for PseudoJet accessors which use internal cached values for efficiency.
For PseudoJet and EEJet update the the constructor from ::Any to test that the LorentzVectorBase interface is supported and construct from those accessors. Tests are added (test-jet-constructors.jl) to check the conversion works and that it also fails with ArgumentError when a type does not support the interface. Test explicitly construction from LorentzVector and LorentzVectorCyl. Documentation on input particles is updated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 81.56% 82.07% +0.50%
==========================================
Files 21 21
Lines 1405 1400 -5
==========================================
+ Hits 1146 1149 +3
+ Misses 259 251 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This source has a lot more in it than just struct definitions, so the name was misleading. Fix a minor inconsistency in the cross constructors for PseudoJet and EEJet.
9cb249b to
7684560
Compare
68e6944 to
bfc0f24
Compare
mmikhasenko
left a comment
There was a problem hiding this comment.
I have not worked with the package yet.
I've looked over the files and have not noticed anything critical to comment.
Most of changes are clear and related to using LorentzVectorBase interface.
Logic related to cluster_hist_index was not very clear to me.
Is it needed to create PseudoJet. In the previous version, there was a constructor without index. Now, index assumed to be 0 if not passed.
Use the feature that bare identifiers imply the keyword argument name.
|
Thank you for looking over @mmikhasenko. I had missed that bare identifiers imply the keyword name. The reason that |
|
I see a small caveat that BTW, is |
|
Thanks for the comment @m-fila. Yes, there is a risk of desynchronisation, but I think it's small. There's probably a case for some internal refactoring to avoid "aliases" and to use For AFAICS you are right about |
For the two main jet types used in the package, implement LorenzVectorBase interfaces by declaring them to be
PxPyPzEfour vectors.Delegate most of the jet property accessors to the LorenzVectorBase implementations, with some "fixup" for phi, rapidity and pseudorapidity:
_MaxRap)This ensures compatibility with previous results from the package.
Improve documentation for those specific accessors which use cached values from our own jets (being more type specialised, these are chosen over the accessors for abstract
FourMomentumtypes).Implement constructors for
PseudoJetandEEJetfrom any types that also implement LorenzVectorBase interfaces. Drop the old generic constructors that usedJetReconstruction.{px,py,pz,energy}()methods - the common LorenzVectorBase interface is the way to go now.Add tests for LorenzVectorBase interface constructors as well as LorenzVector and LorenzVectorCyl.
Update documentation on supported types for the reconstruction methods.
Rename
CommonJetStructs.jltoCommonJet.jlas it contains far more than just struct definitions now.Fix an inconsistency in the construction of
(EEJet, PseudoJet)from(PseudoJet, EEJet)(allowingcluster_hist_indexto be optionally set explicitly).Closes #77