General numeric type support #97
Draft
Conversation
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
Allowing for different Float values, as well as more exotic types, if needed
Allowing for different Float values, as well as more exotic types, if needed.
Ensure that types are stable for EEReco, meaning that they are
EEjet{T} for a specific T, not just generic EEjet
Allow instrumented-jetreco to have a type argument that makes the particle collection have that specific type, allowing for switches to different Floats N.B, Currently Float32 is slower than Float64
It is very important to pass the fully typed PseudoJet{T} around, not a generic PseudoJet
Returns the parametrised element type
Also support type switching constructor for PseudoJet
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
+ Coverage 70.18% 71.30% +1.11%
==========================================
Files 17 17
Lines 1164 1178 +14
==========================================
+ Hits 817 840 +23
+ Misses 347 338 -9 ☔ View full report in Codecov by Sentry. |
|
For completeness, here are the benchmarks on my x86 Ryzen 7 5700G: So, again, the significant performance loss is pp tiled (and a small loss for pp plain). |
m-fila
reviewed
Dec 2, 2024
At the full constructor level the type parameter really should be known, the use case for the non-explicit parametrised version is weak, so it is removed. (This was also causing a docstring warning.)
The particle type is know in the function signature of the underlying reconstruction
We explicitly use Float64 here, because this is deliberately the simple case.
graeme-a-stewart
force-pushed
the
general-numeric-types
branch
from
97046e3 to
faf67cf
Compare
gojakuch
reviewed
Dec 7, 2024
| # If we don't have an algorithm we default to PseudoJet | ||
| if !isnothing(args[:algorithm]) | ||
| JetReconstruction.is_ee(args[:algorithm]) | ||
| jet_type = EEjet{Float64} |
There was a problem hiding this comment.
just a question: why is it explicitly Float64 here, shouldn't this be smth like eval(args[:type]) for generality?
There was a problem hiding this comment.
The reason is that I wanted jetreco.jl to be the simplest and most explicit example possible. instrumented-jetreco.jl is the one with all the bells and whistles.
It might be useful to add a note though, that any subtype of Real is allowed.
| else | ||
| jet_type = PseudoJet | ||
| jet_type = PseudoJet{Float64} |
Numerical type can be passed as a parameter to the test struct This is passed down to read the events in the required precision. Add some tests for e+e- and pp in Float32
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Support is added for general numerical types in this PR.
EEjetandPseudoJetbecome parametrised onT <: Real.ClusterSequencestays parametrised onJ <: FourMomentum, but is it really important to pass the parametrised jet collection into the constructor otherwise performance greatly suffers! (i.e.,ClusterSequence{EEjet{Float64}}notClusterSequence{EEjet}. To support this theeltypemethod is defined for both jet types to return the internal type used.The
instrumented-jetreco.jlscript gets a new--typeoption that allows a numeric type to be passed for the reconstruction. Added a quickbenchmark.shscript to run all three main use cases.Current performance tests shows that we are losing a bit in the$pp$ $e^+e^-$ and $pp$
N2Tiledreconstruction, so this needs to be fixed before merging. Performance for DurhamN2Plainare the same as the current v0.4.3 release.Testing on M2pro gives
This branch:
v0.4.3:
Closes #91
To do:
EEjettoT <: RealPseudoJettoT <: RealTiledJettoT <: Realinstrumented_jetreco.jljetreco.jlFloat64caseFloat32reconstruction