General numeric type support #97
Draft
Conversation
This file contains hidden or 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 73.49% 72.91% -0.59%
==========================================
Files 18 18
Lines 1249 1259 +10
==========================================
Hits 918 918
- Misses 331 341 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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
graeme-a-stewart
force-pushed
the
general-numeric-types
branch
from
97046e3 to
faf67cf
Compare
gojakuch
reviewed
Dec 7, 2024
graeme-a-stewart
force-pushed
the
general-numeric-types
branch
from
9f96393 to
79036ee
Compare
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
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.
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
graeme-a-stewart
force-pushed
the
general-numeric-types
branch
from
79036ee to
a8a9ad4
Compare
Had used wrong comparison - 'isa' instead of '<:'!
graeme-a-stewart
force-pushed
the
general-numeric-types
branch
from
a8a9ad4 to
6150869
Compare
graeme-a-stewart
force-pushed
the
general-numeric-types
branch
2 times, most recently
from
43612fb to
cf7018e
Compare
graeme-a-stewart
force-pushed
the
general-numeric-types
branch
from
cf7018e to
23e7869
Compare
|
With the last set of fixes for correct types comparison, performance for the parameterised types are now the same as for the fixed Fixed: ~/tmp/JetReconstruction.jl/examples/ [main] ./benchmark.sh
pp 14TeV Tiled
Processed 100 events 16 times
Average time per event 180.95424500000001 ± 7.688918170282834 μs
Lowest time per event 171.03375 μs
pp 14 TeV Plain
Processed 100 events 16 times
Average time per event 664.906433125 ± 6.51083132764368 μs
Lowest time per event 658.1575 μs
ee H Durham
Processed 100 events 16 times
Average time per event 27.603828125 ± 0.21974330243013962 μs
Lowest time per event 27.315 μsParameterised: ~/.julia/dev/JetReconstruction/examples/ [general-numeric-types] ./benchmark.sh
pp 14TeV Tiled
Processed 100 events 16 times
Average time per event 177.1443225 ± 6.569998191079404 μs
Lowest time per event 170.38208999999998 μs
pp 14 TeV Plain
Processed 100 events 16 times
Average time per event 664.585339375 ± 4.866904941313732 μs
Lowest time per event 659.27709 μs
ee H Durham
Processed 100 events 16 times
Average time per event 27.670936875 ± 0.31133985727780145 μs
Lowest time per event 27.28125 μs |
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