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

More cosmetic and usability changes #25

Merged
merged 1 commit into from
Jul 28, 2024
Merged

Conversation

peremato
Copy link
Member

Added extra usability functions:

  • GetObject(file::Union{ROOT.TDirectoryFile, CxxPtr{<:ROOT.TDirectoryFile}}, name::AbstractString)
  • Base.getproperty(file::Union{ROOT.TDirectoryFile, CxxPtr{<:ROOT.TDirectoryFile}}, sym::Symbol)

This allows to get all sort of objects from a TFile

julia> f = ROOT.TFile!Open("demo_ROOT_out.root");

julia> @test GetObject(f,"h") isa ROOT.TH1
Test Passed

julia> f = ROOT.TFile!Open("test1.root")
CxxWrap.CxxWrapCore.CxxPtr{ROOT.TFile}(Ptr{ROOT.TFile} @0x000000012c833d20)

julia> @test f.tree isa ROOT.TTree
Test Passed

@peremato peremato requested a review from grasph June 13, 2024 09:42
Copy link
Member

@grasph grasph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pere for this nice PR. It's great you find how to implement a generic get function to retrieve objects from the TFile. I've added some comments about improvements.

examples/TTree_examples/read_tree1.jl Show resolved Hide resolved
examples/TTree_examples/write_tree1.jl Show resolved Hide resolved
brMuon_pt = Branch(tree, "Muon_pt[nMuon]", Muon_pt, 32000, 99)
brMuon_eta = Branch(tree, "Muon_eta[nMuon]", Muon_eta, 32000, 99)
brMuon_phi = Branch(tree, "Muon_phi[nMuon]", Muon_phi, 32000, 99)
nMuon = Ref{Int32}(0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the purpose of the change. We don't necessarily know in advance the maximum size of a collection.

The example was showing how to do in case of variable length collection.

I think the original example should be kept.

src/demo.jl Outdated
@@ -13,6 +13,6 @@ function demo()
FillRandom(h, "gaus")
c = ROOT.TCanvas()
Fit(h, "gaus")
c
Draw(c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to return 'c' to keep a reference to the TCanvas, which otherwise can be garbage collected at any time.

Why the call to 'Draw()'? Call to 'Fit' already triggers a draw, at least on Linux. Does the behaviour differ on mac (I don't have the possibility to test it)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored original code. Confirmation that the canvas is plotted on MacOS is needed.

src/ROOTex.jl Outdated Show resolved Hide resolved
src/ROOTex.jl Outdated Show resolved Hide resolved
examples/TTree_examples/read_tree1.jl Outdated Show resolved Hide resolved
@grasph
Copy link
Member

grasph commented Jun 27, 2024

@peremato can you review the changes I made and tell me if they are ok for you?

Project.toml Outdated Show resolved Hide resolved
@grasph grasph force-pushed the usability-3 branch 2 times, most recently from fc474b8 to eb75743 Compare July 27, 2024 21:45
@grasph grasph force-pushed the master branch 3 times, most recently from 9ecc35f to 7d0a95b Compare July 28, 2024 11:41
…File.

The methods returns now an instance of the object, of proper type, instead of
a Ptr{TObjet}.

getproperty is also implemented with the same effect as the Get method, to
provide object access with the dot notation, `file.objet`.

Code from commit c10c8e028cc29837f765aeeb4cb717c8c45392e4 of ROOT.jl-generator

Co-authored-by: Philippe Gras <[email protected]>
@grasph grasph merged commit 1fbf9a5 into JuliaHEP:master Jul 28, 2024
5 checks passed
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