Skip to content
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

fix Vector{Bool} branches and factorize interped_data #95

Closed
wants to merge 5 commits into from

Conversation

aminnj
Copy link
Member

@aminnj aminnj commented Sep 10, 2021

The change in #93 results in BitVector instead of Vector{Bool} which breaks LazyBranch. We didn't have a test for it, but I added one here. I split up the large interped_data into smaller chunks for non-, singly-, doubly-jagged branches so that it's easier to specialize on Bool. Evidently this breaks some LV64 test cases which I'm still figuring out.

TLorentzVector: Error During Test at /Users/namin/.julia/dev/UnROOT/test/runtests.jl:238
  Test threw exception
  Expression: (branch[1]).x == 1.0
  MethodError: interped_data(::Vector{UInt8}, ::Vector{Int32}, ::Type{LorentzVector{Float64}}, ::Type{UnROOT.Nojagg}) is ambiguous. Candidates:
    interped_data(rawdata, rawoffsets, ::Type{T}, ::Type{UnROOT.Nojagg}) where T in UnROOT at /Users/namin/.julia/dev/UnROOT/src/root.jl:196
    interped_data(rawdata, rawoffsets, ::Type{LorentzVector{Float64}}, ::Type{J}) where {T, J<:UnROOT.JaggType} in UnROOT at /Users/namin/.julia/dev/UnROOT/src/custom.jl:76
  Possible fix, define
    interped_data(::Any, ::Any, ::Type{LorentzVector{Float64}}, ::Type{UnROOT.Nojagg})
  Stacktrace:
   [1] basketarray(f::ROOTFile, branch::UnROOT.TBranchElement_10, ithbasket::Int64)
     @ UnROOT ~/.julia/dev/UnROOT/src/iteration.jl:57

@aminnj
Copy link
Member Author

aminnj commented Sep 10, 2021

Alternatively, I could just undo the factorization of interpred_data and simply add

function interped_data(rawdata, rawoffsets, ::Type{T}, ::Type{Nojagg}) where {T<:Bool}
    return map(ntoh,reinterpret(T, rawdata))
end

but then it's less nice to read the long if-else chain.

@aminnj
Copy link
Member Author

aminnj commented Sep 10, 2021

Since the typing in root.jl isn't a catch-all, I restricted the typing of various things in custom.jl. Unit tests pass, but @Moelf @tamasgal can you make sure I didn't do anything destructive? If it's better, I'll just do what I wrote in my previous message.

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #95 (04bcbe2) into master (60986a5) will increase coverage by 0.02%.
The diff coverage is 92.72%.

❗ Current head 04bcbe2 differs from pull request most recent head 2862c3f. Consider uploading reports for the commit 2862c3f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   84.61%   84.63%   +0.02%     
==========================================
  Files          11       11              
  Lines        1287     1328      +41     
==========================================
+ Hits         1089     1124      +35     
- Misses        198      204       +6     
Impacted Files Coverage Δ
src/custom.jl 69.56% <50.00%> (-2.53%) ⬇️
src/root.jl 91.07% <100.00%> (+0.41%) ⬆️
src/iteration.jl 70.07% <0.00%> (-0.39%) ⬇️
src/streamers.jl 91.12% <0.00%> (-0.08%) ⬇️
src/utils.jl 100.00% <0.00%> (ø)
src/types.jl 92.10% <0.00%> (+0.10%) ⬆️
src/bootstrap.jl 83.56% <0.00%> (+0.23%) ⬆️
src/displays.jl 50.00% <0.00%> (+1.42%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60986a5...2862c3f. Read the comment docs.

@aminnj aminnj changed the title [WIP] fix Vector{Bool} branches fix Vector{Bool} branches and factorize interped_data Sep 10, 2021
@Moelf
Copy link
Member

Moelf commented Sep 10, 2021

Tamas probably uses more custom.jl than I do, maybe he will have some input as an expert user.

@tamasgal
Copy link
Member

I am back on track. Thanks for this PR, I will check it and report back, but I think it should be fine since the test coverage is pretty good for custom stuff. That's why I wanted to leave it there in first place ;)

@tamasgal
Copy link
Member

I tried it but I get ambiguities. I probably need to change the way I deal with interped_data():

MethodError: interped_data(::Vector{UInt8}, ::Vector{Int32}, ::Type{NeRCA.KM3NETDAQEventHeader}, ::Type{UnROOT.Nojagg}) is ambiguous. Candidates:
  interped_data(rawdata, rawoffsets, ::Type{T}, ::Type{UnROOT.Nojagg}) where T in UnROOT at /home/tgal/Dev/UnROOT.jl/src/root.jl:196
  interped_data(rawdata, rawoffsets, ::Type{NeRCA.KM3NETDAQEventHeader}, ::Type{J}) where {T, J<:UnROOT.JaggType} in NeRCA at /home/tgal/Dev/NeRCA.jl/src/io/root.jl:67
Possible fix, define
  interped_data(::Any, ::Any, ::Type{NeRCA.KM3NETDAQEventHeader}, ::Type{UnROOT.Nojagg})

Stacktrace:
  [1] basketarray(f::ROOTFile, branch::UnROOT.TBranchElement_10, ithbasket::Int64)
    @ UnROOT ~/Dev/UnROOT.jl/src/iteration.jl:57
  [2] getindex(ba::LazyBranch{NeRCA.KM3NETDAQEventHeader, UnROOT.Nojagg, Vector{NeRCA.KM3NETDAQEventHeader}}, idx::Int64)
    @ UnROOT ~/Dev/UnROOT.jl/src/iteration.jl:152
  [3] iterate
    @ ~/Dev/UnROOT.jl/src/iteration.jl:166 [inlined]
  [4] iterate
    @ ~/Dev/UnROOT.jl/src/iteration.jl:165 [inlined]
  [5] copyto_unaliased!(deststyle::IndexLinear, dest::Vector{NeRCA.KM3NETDAQEventHeader}, srcstyle::IndexCartesian, src::LazyBranch{NeRCA.KM3NETDAQEventHeader, UnROOT.Nojagg, Vector{NeRCA.KM3NETDAQEventHeader}})
    @ Base ./abstractarray.jl:975
  [6] copyto!
    @ ./abstractarray.jl:950 [inlined]
  [7] copyto_axcheck!
    @ ./abstractarray.jl:1056 [inlined]
  [8] Array
    @ ./array.jl:540 [inlined]
  [9] convert(#unused#::Type{Vector{NeRCA.KM3NETDAQEventHeader}}, a::LazyBranch{NeRCA.KM3NETDAQEventHeader, UnROOT.Nojagg, Vector{NeRCA.KM3NETDAQEventHeader}})
    @ Base ./array.jl:532
 [10] OnlineFile(filename::String)
    @ NeRCA ~/Dev/NeRCA.jl/src/io/root.jl:91
 [11] reco(fname::String, calib::Calibration, sparams::SingleDURecoParams, outfile::String; n_events::Int64)
    @ Main ./In[5]:6
 [12] top-level scope
    @ ./In[6]:19
 [13] eval
    @ ./boot.jl:360 [inlined]
 [14] include_string(mapexpr::typeof(REPL.softscope), mod::Module, code::String, filename::String)
    @ Base ./loading.jl:1094

@tamasgal
Copy link
Member

Ah wait, I am dumb. This is colliding with an existing implementation in the core library...

@tamasgal
Copy link
Member

OK, I refactored custom.jl a little bit and added more tests for full coverage. Can you rebase onto master?

@aminnj
Copy link
Member Author

aminnj commented Sep 15, 2021

I screwed up the merge, so I replaced this PR with #106 . ;)

@aminnj aminnj closed this Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants