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

Reading of c-arrays + more customstructs #167

Closed
wants to merge 5 commits into from

Conversation

briederer
Copy link

After prior discussion in #165 and #166 respectively I decided to put a little bit of effort into extending it to fix #9 finally.

For that I simply enhanced the customstruct to an arbitrary dimensional FixSizeArray and parse the fLeaf.fTitle for the decomposition. Everything else works as expected.

Two caveats:

  1. Since all of it relies on wrapping the StaticArrays.SArray struct nicely we are also limited to there specifications. With the current implementation of the struct Base.sizeof() does not work out of the box due to a missing 4.th parameter in the SArray-type. (see: https://github.com/briederer/UnROOT.jl/blob/3785a042a9cd9855e5f0b9ee18e3b89448bb15d3/src/custom.jl#L49-L53). This means we either add an (for this purpose) unnecessary parameter to the struct or we simply overwrite the Base.sizeof method as is done here.
  2. I've also added (like it is usually done in Julia) aliases for FixSizeVector and FixSizeMatrix to improve readability in the output. However due to some restrictions (see Type aliases are not printed as aliases if the aliased type is imported from a different module JuliaLang/julia#40448) the aliases would only be seen for the User if the additional types are exported. But this is not my choice 😉

Closes #9

@Moelf
Copy link
Member

Moelf commented May 5, 2022

julia> a = @SMatrix [1 2 3; 4 5 6]
2×3 SMatrix{2, 3, Int64, 6} with indices SOneTo(2)×SOneTo(3):
 1  2  3
 4  5  6

julia> sizeof(a)
48

julia> sizeof(typeof(a))
48

it should work?

@Moelf
Copy link
Member

Moelf commented May 5, 2022

I forgot why we can't use SArray directly actually, I read the last discussion, it seems to be because we build the buffer by calling the constructor:

_buffer = VectorOfVectors(T(), Int32[1])

and of course the StaticArray doesn't have constructor. But we can work around this by defining our own:

_initialize(::Type{T}) = T()
_initialize(::Type{T<:SArray}) = zero(T)

this way we don't have to wrap them?

@briederer
Copy link
Author

Indeed in this case it works. But this is due to the fact, that the actual type of your SMatrix is SMatrix{2, 3, Int64, 6} which is an alias for SArray{Tuple{2, 3}, Int64, 2, 6}.
However the inner type in "my" FixSizeArray is missing the last entry in the type arguments (i.e. it is SArray{Tuple{...}, T, 2} which is basically a subtype since the last argument can be deduced from the product of all Tuple{...} entries. So in fact the wrapped struct is not exactly like in your example.
However it could be adapted by adding everywhere this 4th argument, which seems unnecessary to me though.

@briederer
Copy link
Author

briederer commented May 5, 2022

I forgot why we can't use SArray directly actually, I read the last discussion, it seems to be because we build the buffer by calling the constructor:

_buffer = VectorOfVectors(T(), Int32[1])

and of course the StaticArray doesn't have constructor. But we can work around this by defining our own:

_initialize(::Type{T}) = T()
_initialize(::Type{T<:SArray}) = zero(T)

this way we don't have to wrap them?

I am not sure this would work?
First problem is that according to the last discussion the arrays are Nojagg and thus would not even enter this part of the code. However even if you circumvent this problem I am not sure if calling a VectorOfVectors(StaticArrays(),...) wouldn't be a bit of an overkill (too many nested vector-like structs).
But I agree that getting rid off the wrapper would be a good thing.

@briederer briederer changed the title Reading of c-arrays Reading of c-arrays + more customstructs May 5, 2022
@briederer
Copy link
Author

I've added now also the possibility to allwo the customstructs dict to allow for special types simply by branchnames.

This enables a bit more customizability if someone needs special interpretation of some branches.
E.g. i've got one branch called complex[2]/D containing complex numbers stored as two entries. Since it is not a C++-class the branch has no fClassName field and the usual approach does not work. Therefore searching for names too is a possible workaround.

@briederer
Copy link
Author

briederer commented May 5, 2022

To be honest I just realised that the last addition is everything that I need for any of my "special branches".

So if you think that the other parts of this PR (and what has been in added in #166) adds too much overhead/unnecessary stuff I would be totally fine with removing all of that. As long as defining "customstructs" also via fTitle (or fName which is probably more intuitive) remains in this PR I am happy 😊

@briederer
Copy link
Author

Btw. why are tests failing?
I can't access the CI-logs so I don't now what's happening there. Will check if tests fail locally too, tomorrow.

@Moelf
Copy link
Member

Moelf commented May 5, 2022

@Moelf
Copy link
Member

Moelf commented May 5, 2022

does test pass for you locally? ]test UnROOT

@briederer
Copy link
Author

Well they are accessible but it says "The log was not found" 🤔
image

@briederer
Copy link
Author

does test pass for you locally? ]test UnROOT

Will try it tomorrow.

@briederer
Copy link
Author

briederer commented May 6, 2022

Tried now the tests locally. The failing one is this:

UnROOT.jl/test/runtests.jl

Lines 733 to 738 in 7d37122

@testset "SourceStream remote" begin
t = LazyTree("https://scikit-hep.org/uproot3/examples/Zmumu.root", "events")
@test t.eta1[1] -1.21769
@test t.eta1[end] -1.57044
show(devnull, t) # test display
end

It seems that this file has a TBranch called "Type" populated with TLeafC which is a variable size string. Problem that occurs is that this has a leaf.fLen > 1 and thus enters the condition added in #166. Before my current modification (i.e. extending to arbitrary size arrays) the test did work because it simply reinterpreted the string wrong (as a FixLenVector instead of a string) which is not covered by the tests.

Fixed it by simply leaving it as FixSizeVector{leaf.fLen,UInt8} which is a "untranslated" vector of chars. I didn't manage to adopt the output to produce a string for each event (problem with no definite size-strings when reinterpreting) but I guess this could be adapted. I.e. it simply would need another wrapping before being printed like String.(FixSizeVector{leaf.fLen,UInt8})

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #167 (2e2b444) into master (7d37122) will decrease coverage by 0.41%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
- Coverage   90.71%   90.29%   -0.42%     
==========================================
  Files          11       11              
  Lines        1561     1566       +5     
==========================================
- Hits         1416     1414       -2     
- Misses        145      152       +7     
Impacted Files Coverage Δ
src/UnROOT.jl 66.66% <ø> (-23.34%) ⬇️
src/custom.jl 97.89% <92.30%> (+0.02%) ⬆️
src/iteration.jl 88.94% <100.00%> (ø)
src/root.jl 93.82% <100.00%> (+0.02%) ⬆️
src/utils.jl 100.00% <100.00%> (ø)
src/streamers.jl 89.83% <0.00%> (-1.70%) ⬇️

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 7d37122...2e2b444. Read the comment docs.

@Moelf
Copy link
Member

Moelf commented Jun 1, 2022

looks like it's passing for >=1.6, I will give it a read later and see if we can manage without so much custom type

@Moelf
Copy link
Member

Moelf commented Jan 7, 2023

this is partly addressed in #188

@briederer
Copy link
Author

Sadly it appears I'll never find time to finish this and (probably) won't need it any time in the near future.
Therefore I am closing the PR now.
In any case someone wants to pick it up again, feel free. I will not delete the branch.

@briederer briederer closed this Mar 15, 2023
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.

Reading structs into higher-dimensional Arrays
2 participants