Fix streamer lookup #227

tamasgal · 2023-03-15T10:27:58Z

@Moelf I might need a little bit of help here ;) I realised that the streamer lookup is bugged and I also need to change the order of decisions in JaggType. I think that streamer information should have precedence over other methods of type-guess, like matching [*] in the title etc.

Currently we have a "last-resort" lookup in the streamer list of a class, where we take the first element. This was calling for trouble for custom classes which have multiple fields (with possibly different types). See my revelation at #226 (comment)

TL;DR: I realised that fID is the index of to the streamer of the parent class' streamer list, so this PR is definitely a bug fix.

All the tests pass but two are failing:

issues: Error During Test at /Users/tamasgal/Dev/UnROOT.jl/test/runtests.jl:620
  Test threw exception
  Expression: (LazyBranch(rootfile, "Events/Jet_pt"))[:] == Vector{Float32}[[], [27.324587, 24.889547, 20.853024], [], [20.33066], [], []]
  MethodError: Cannot `convert` an object of type Float32 to an object of type Vector{Float32}
  Closest candidates are:
    convert(::Type{T}, ::LinearAlgebra.Factorization) where T<:AbstractArray at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/LinearAlgebra/src/factorization.jl:58
    convert(::Type{Array{T, N}}, ::SizedArray{S, T, N, N, Array{T, N}}) where {S, T, N} at ~/.julia/packages/StaticArrays/PUoe1/src/SizedArray.jl:88
    convert(::Type{Array{T, N}}, ::SizedArray{S, T, N, M, TData} where {M, TData<:AbstractArray{T, M}}) where {T, S, N} at ~/.julia/packages/StaticArrays/PUoe1/src/SizedArray.jl:82
    ...
  Stacktrace:
    [1] setindex!(A::Vector{Vector{Float32}}, x::Float32, i1::Int64)
      @ Base ./array.jl:966
    [2] _unsafe_copyto!(dest::Vector{Vector{Float32}}, doffs::Int64, src::Vector{Float32}, soffs::Int64, n::Int64)
      @ Base ./array.jl:253
    [3] unsafe_copyto!
      @ ./array.jl:307 [inlined]
    [4] _copyto_impl!
      @ ./array.jl:331 [inlined]
    [5] copyto!
      @ ./array.jl:317 [inlined]
    [6] copyto!
      @ ./array.jl:343 [inlined]
    [7] copyto_axcheck!
      @ ./abstractarray.jl:1127 [inlined]
    [8] Array
      @ ./array.jl:626 [inlined]
    [9] convert
      @ ./array.jl:617 [inlined]
   [10] setindex!(A::Vector{Vector{Vector{Float32}}}, x::Vector{Float32}, i1::Int64)
      @ Base ./array.jl:966
   [11] _localindex_newbasket!(ba::LazyBranch{Vector{Float32}, UnROOT.Nojagg, Vector{Vector{Float32}}}, idx::Int64, tid::Int64)
      @ UnROOT ~/Dev/UnROOT.jl/src/iteration.jl:183
   [12] getindex
      @ ~/Dev/UnROOT.jl/src/iteration.jl:174 [inlined]
   [13] macro expansion
      @ ./multidimensional.jl:903 [inlined]
   [14] macro expansion
      @ ./cartesian.jl:64 [inlined]
   [15] _unsafe_getindex!
      @ ./multidimensional.jl:898 [inlined]
   [16] _unsafe_getindex(#unused#::IndexLinear, A::LazyBranch{Vector{Float32}, UnROOT.Nojagg, Vector{Vector{Float32}}}, I::Base.Slice{Base.OneTo{Int64}})
      @ Base ./multidimensional.jl:889
   [17] _getindex
      @ ./multidimensional.jl:875 [inlined]
   [18] getindex(A::LazyBranch{Vector{Float32}, UnROOT.Nojagg, Vector{Vector{Float32}}}, I::Function)
      @ Base ./abstractarray.jl:1241
   [19] macro expansion
      @ ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Test/src/Test.jl:464 [inlined]
   [20] macro expansion
      @ ~/Dev/UnROOT.jl/test/runtests.jl:620 [inlined]
   [21] macro expansion
      @ ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Test/src/Test.jl:1363 [inlined]
   [22] top-level scope
      @ ~/Dev/UnROOT.jl/test/runtests.jl:601
issues: Error During Test at /Users/tamasgal/Dev/UnROOT.jl/test/runtests.jl:600
  Got exception outside of a @test
  MethodError: Cannot `convert` an object of type Float32 to an object of type Vector{Float32}
  Closest candidates are:
    convert(::Type{T}, ::LinearAlgebra.Factorization) where T<:AbstractArray at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/LinearAlgebra/src/factorization.jl:58
    convert(::Type{Array{T, N}}, ::SizedArray{S, T, N, N, Array{T, N}}) where {S, T, N} at ~/.julia/packages/StaticArrays/PUoe1/src/SizedArray.jl:88
    convert(::Type{Array{T, N}}, ::SizedArray{S, T, N, M, TData} where {M, TData<:AbstractArray{T, M}}) where {T, S, N} at ~/.julia/packages/StaticArrays/PUoe1/src/SizedArray.jl:82
    ...
  Stacktrace:
    [1] setindex!(A::Vector{Vector{Float32}}, x::Float32, i1::Int64)
      @ Base ./array.jl:966
    [2] _unsafe_copyto!(dest::Vector{Vector{Float32}}, doffs::Int64, src::Vector{Float32}, soffs::Int64, n::Int64)
      @ Base ./array.jl:253
    [3] unsafe_copyto!
      @ ./array.jl:307 [inlined]
    [4] _copyto_impl!
      @ ./array.jl:331 [inlined]
    [5] copyto!
      @ ./array.jl:317 [inlined]
    [6] copyto!
      @ ./array.jl:343 [inlined]
    [7] copyto_axcheck!
      @ ./abstractarray.jl:1127 [inlined]
    [8] Array
      @ ./array.jl:626 [inlined]
    [9] convert
      @ ./array.jl:617 [inlined]
   [10] setindex!(A::Vector{Vector{Vector{Float32}}}, x::Vector{Float32}, i1::Int64)
      @ Base ./array.jl:966
   [11] _localindex_newbasket!(ba::LazyBranch{Vector{Float32}, UnROOT.Nojagg, Vector{Vector{Float32}}}, idx::Int64, tid::Int64)
      @ UnROOT ~/Dev/UnROOT.jl/src/iteration.jl:183
   [12] getindex
      @ ~/Dev/UnROOT.jl/src/iteration.jl:174 [inlined]
   [13] _broadcast_getindex
      @ ./broadcast.jl:636 [inlined]
   [14] _getindex
      @ ./broadcast.jl:667 [inlined]
   [15] _broadcast_getindex
      @ ./broadcast.jl:642 [inlined]
   [16] getindex
      @ ./broadcast.jl:597 [inlined]
   [17] macro expansion
      @ ./broadcast.jl:961 [inlined]
   [18] macro expansion
      @ ./simdloop.jl:77 [inlined]
   [19] copyto!
      @ ./broadcast.jl:960 [inlined]
   [20] copyto!
      @ ./broadcast.jl:913 [inlined]
   [21] copy
      @ ./broadcast.jl:885 [inlined]
   [22] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(length), Tuple{LazyBranch{Vector{Float32}, UnROOT.Nojagg, Vector{Vector{Float32}}}}})
      @ Base.Broadcast ./broadcast.jl:860
   [23] macro expansion
      @ ~/Dev/UnROOT.jl/test/runtests.jl:626 [inlined]
   [24] macro expansion
      @ ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/Test/src/Test.jl:1363 [inlined]
   [25] top-level scope
      @ ~/Dev/UnROOT.jl/test/runtests.jl:601
   [26] include(fname::String)
      @ Base.MainInclude ./client.jl:476
   [27] top-level scope
      @ none:6
   [28] eval
      @ ./boot.jl:368 [inlined]
   [29] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:276
   [30] _start()
      @ Base ./client.jl:522
Test Summary: | Pass  Error  Total  Time
issues        |    7      2      9  1.9s
ERROR: LoadError: Some tests did not pass: 7 passed, 0 failed, 2 errored, 0 broken.
in expression starting at /Users/tamasgal/Dev/UnROOT.jl/test/runtests.jl:600
ERROR: Package UnROOT errored during testing

tamasgal · 2023-03-15T10:30:33Z

Here is the actual MWE:

julia> using UnROOT

julia> rootfile = UnROOT.samplefile("issue61.root")
ROOTFile with 1 entry and 18 streamers.
/Users/tamasgal/Dev/UnROOT.jl/test/samples/issue61.root
└─ Events (TTree)
   └─ "Jet_pt"


julia> LazyBranch(rootfile, "Events/Jet_pt")
6-element LazyBranch{Vector{Float32}, UnROOT.Nojagg, Vector{Vector{Float32}}}: 
Error showing value of type LazyBranch{Vector{Float32}, UnROOT.Nojagg, Vector{Vector{Float32}}}:
ERROR: MethodError: Cannot `convert` an object of type Float32 to an object of type Vector{Float32}

tamasgal · 2023-03-15T10:38:54Z

Apparently there is a streamer for vector<float> in the lookup

julia> UnROOT.streamerfor(rootfile, "vector<float>")
UnROOT.StreamerInfo(UnROOT.TStreamerInfo{UnROOT.TObjArray}("vector<float>", "", 0x055a169b, 6, UnROOT.TObjArray("", 0, Any[UnROOT.TStreamerSTL
  version: UInt16 0x0004
  fOffset: Int64 0
  fName: String "This"
  fTitle: String "<Float_t> Used to call the proper TStreamerInfo case"
  fType: Int32 500
  fSize: Int32 0
  fArrayLength: Int32 0
  fArrayDim: Int32 0
  fMaxIndex: Array{Int32}((5,)) Int32[0, 0, 0, 0, 0]
  fTypeName: String "vector<float>"
  fXmin: Float64 0.0
  fXmax: Float64 0.0
  fFactor: Float64 0.0
  fSTLtype: Int32 1
  fCtype: Int32 5
])), Set{Any}())

but the fID is -1. I can't make any sense of that, I'd expect 0 so that we get the correct streamer at index 1, but with -1 we land at 0, which gives an index error and then returns NoJagg.

I am a bit lost and have not found anything about it yet.

julia> rootfile["Events/Jet_pt"]
UnROOT.TBranchElement_10
  cursor: UnROOT.Cursor
  fName: String "Jet_pt"
  fTitle: String "Jet_pt"
  fFillColor: Int16 0
  fFillStyle: Int16 1001
  fCompress: Int32 1
  fBasketSize: Int32 32000
  fEntryOffsetLen: Int32 24
  fWriteBasket: Int32 1
  fEntryNumber: Int64 6
  fIOFeatures: UnROOT.ROOT_3a3a_TIOFeatures
  fOffset: Int32 0
  fMaxBaskets: UInt32 0x0000000a
  fSplitLevel: Int32 99
  fEntries: Int64 6
  fFirstEntry: Int64 0
  fTotBytes: Int64 183
  fZipBytes: Int64 167
  fBranches: UnROOT.TObjArray
  fLeaves: UnROOT.TObjArray
  fBaskets: UnROOT.TObjArray
  fBasketBytes: Array{Int64}((10,)) [167, 0, 0, 0, 0, 0, 0, 0, 0, 0]
  fBasketEntry: Array{Int64}((10,)) [0, 6, 0, 0, 0, 0, 0, 0, 0, 0]
  fBasketSeek: Array{Int64}((10,)) [226, 0, 0, 0, 0, 0, 0, 0, 0, 0]
  fFileName: String ""
  fClassName: String "vector<float>"
  fParentName: String ""
  fClonesName: String ""
  fCheckSum: UInt32 0x055a169b
  fClassVersion: Int16 6
  fID: Int32 -1
  fType: Int32 0
  fStreamerType: Int32 -1
  fMaximum: Int32 0
  fBranchCount: Missing missing
  fBranchCount2: Missing missing

Apologise for desperately pinging you @jpivarski but maybe you have a hint ;) I know that you are busy with other things so feel free to ignore this silently!

Moelf · 2023-03-15T13:29:45Z

never liked type guessing so this is great!

Moelf

feel free to merge as long as tests are passing

tamasgal · 2023-03-15T13:32:42Z

So for now I am just forcing it to be 0 if it's -1. The test suite works.

Btw. Jerry asked ChatGPT and here is the answer 😆

Here the transcript:

In the CERN ROOT TBranch class, the fID variable is an integer that represents the unique identifier of a TBranch object. When fID is equal to -1, it means that the TBranch object has not been registered yet in the TTree's list of branches. This can happen, for example, when a TBranch object has been created, but has not been added to a TTree with the TTree::Branch() method.

codecov · 2023-03-15T13:33:45Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (3c2ad37) 90.11% compared to head (dbbce9c) 90.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
+ Coverage   90.11%   90.12%   +0.01%     
==========================================
  Files          18       18              
  Lines        2124     2127       +3     
==========================================
+ Hits         1914     1917       +3     
  Misses        210      210

Impacted Files	Coverage Δ
src/utils.jl	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tamasgal · 2023-03-15T13:38:15Z

Alight, I added some notes. It's yet unclear how to treat it correctly but for now fID = 0 is working for the test suite.

Let's merge and come back if someone complains `;)

Fix streamer lookup

a0c7aac

Preliminary fix with some mysteries about fID=-1

f15dc6d

tamasgal marked this pull request as ready for review March 15, 2023 13:25

tamasgal requested a review from Moelf March 15, 2023 13:26

Moelf approved these changes Mar 15, 2023

View reviewed changes

Add more information to fID=-1

dbbce9c

tamasgal merged commit 146fe24 into master Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix streamer lookup #227

Fix streamer lookup #227

tamasgal commented Mar 15, 2023

tamasgal commented Mar 15, 2023

tamasgal commented Mar 15, 2023

Moelf commented Mar 15, 2023

Moelf left a comment

tamasgal commented Mar 15, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

tamasgal commented Mar 15, 2023

Fix streamer lookup #227

Fix streamer lookup #227

Conversation

tamasgal commented Mar 15, 2023

tamasgal commented Mar 15, 2023

tamasgal commented Mar 15, 2023

Moelf commented Mar 15, 2023

Moelf left a comment

Choose a reason for hiding this comment

tamasgal commented Mar 15, 2023 • edited Loading

codecov bot commented Mar 15, 2023 • edited Loading

Codecov Report

tamasgal commented Mar 15, 2023

tamasgal commented Mar 15, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading