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

Add Iterator interface fix #6 #28

Merged
merged 20 commits into from
Jul 8, 2021
Merged

Add Iterator interface fix #6 #28

merged 20 commits into from
Jul 8, 2021

Conversation

Moelf
Copy link
Member

@Moelf Moelf commented Jul 7, 2021

The code is a bit messy but the idea is there. I will clean up a bit before merging just want to know if people have thoughts on the implementation. The speed seems okay, allocation probably can be reduced, but a revamp on the interped_data() would benefit all use case.

image

It's thread-safe to iterator through multiple branches by simply zipping them together:

julia> rf = ROOTFile("./tree_with_large_array.root")
ROOTFile("./tree_with_large_array.root") with 1 entry and 17 streamers.

julia> it1 = BranchItr(rf, rf["t1/int32_array"])
itBranchItr{Int32, UnROOT.Nojagg}
Branch: int32_array
Total entry: 100000

julia> it2 = BranchItr(rf, rf["t1/float_array"])
BranchItr{Float32, UnROOT.Nojagg}
Branch: float_array
Total entry: 100000

julia> for (a,b) in zip(it1,it2)
           a, b
       end

@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #28 (271002f) into master (a975f22) will decrease coverage by 0.07%.
The diff coverage is 79.06%.

❗ Current head 271002f differs from pull request most recent head bd40294. Consider uploading reports for the commit bd40294 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   81.58%   81.50%   -0.08%     
==========================================
  Files           8        9       +1     
  Lines         885      930      +45     
==========================================
+ Hits          722      758      +36     
- Misses        163      172       +9     
Impacted Files Coverage Δ
src/UnROOT.jl 100.00% <ø> (ø)
src/arrayapi.jl 74.46% <74.46%> (ø)
src/bootstrap.jl 79.56% <80.00%> (+<0.01%) ⬆️
src/streamers.jl 88.88% <80.00%> (-0.25%) ⬇️
src/root.jl 79.16% <83.33%> (+1.96%) ⬆️
src/types.jl 92.40% <87.50%> (-1.11%) ⬇️
src/utils.jl 100.00% <100.00%> (ø)

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 a975f22...bd40294. Read the comment docs.

@Moelf
Copy link
Member Author

Moelf commented Jul 7, 2021

@aminnj mentioned that when iterating over a (wide) tree, one should be lazy about unpacking baskets because there could be a continue branch in the event-loop that is executed 99.99% of the time (so you can ignore all other O(100) branches for almost all the time).

We don't have a tree iterator interface yet and this current design is hard to fit the bill, any suggestion would be welcomed.

Moelf and others added 2 commits July 8, 2021 12:40
@Moelf
Copy link
Member Author

Moelf commented Jul 8, 2021

image
I combined the indexing and iteration interface, such that we support broadcast and map over a LazyBranch

@tamasgal
Copy link
Member

tamasgal commented Jul 8, 2021

Looks very good!

[skip ci]
@Moelf Moelf merged commit 0338002 into JuliaHEP:master Jul 8, 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.

2 participants