rootio: introduce a chain of Trees #162
Conversation
sbinet
left a comment
sbinet
left a comment
There was a problem hiding this comment.
thanks!
it's a good first step.
I do have a few comments, see below.
Also, please add a test for Chain, in a tchain_test.go file.
You can use the testdata/chain*.root files for that test (e.g. you can test that Entries() returns the correct number of entries, and test for Name() and Title().)
| @@ -0,0 +1,115 @@ | |||
| package rootio | |||
There was a problem hiding this comment.
this needs a license header.
see rootio.go for an example. (change the year to the current one, 2018)
rootio/tchain.go
Outdated
| } | ||
|
|
||
| func (t tchain) Entries() int64 { | ||
| var v int64 = 0 |
There was a problem hiding this comment.
you can drop the = 0 part.
in Go, all values which are not initialized get a default zero value.
that zero value depends on the type, of course (e.g. for a string, that's "", for a pointer it's nil, for a float64, it's 0.0, etc...)
rootio/tchain.go
Outdated
| v = v + t.trees[i].Entries() | ||
| } | ||
| return v | ||
|
|
rootio/tchain.go
Outdated
| func (t tchain) Entries() int64 { | ||
| var v int64 = 0 | ||
| for i := range t.trees { | ||
| v = v + t.trees[i].Entries() |
There was a problem hiding this comment.
you could use the += operator here.
v += t.trees[i].Entries()
rootio/tchain.go
Outdated
|
|
||
| func (t tchain) Entries() int64 { | ||
| var v int64 = 0 | ||
| for i := range t.trees { |
There was a problem hiding this comment.
here, we aren't really interested in the index into that slice of trees: we just use the indexed value.
so we can use this form of range:
for _, tree := range t.trees {
v += tree.Entries()
}
rootio/tchain.go
Outdated
| //Methods of Named interface : | ||
|
|
||
| func (t tchain) Name() string { | ||
| return t.trees[0].Name() |
There was a problem hiding this comment.
same comment than for Branches().
rootio/tchain.go
Outdated
| } | ||
|
|
||
| func (t tchain) Title() string { | ||
| return t.trees[0].Title() |
There was a problem hiding this comment.
same comment than for Branches().
rootio/tchain.go
Outdated
|
|
||
| //Methods of Named interface : | ||
|
|
||
| func (t tchain) Name() string { |
There was a problem hiding this comment.
please move the implementation of that method earlier in that file.
I like to have them more or less in the order they are laid out in the interface.
tchain implements rootio.Object, rootio.Named and rootio.Tree.
so we should see first the implementation of the methods for these interfaces in that order.
rootio/tchain.go
Outdated
| } | ||
|
|
||
| var ( | ||
| _ Tree = (*tchain)(nil) |
There was a problem hiding this comment.
tchain is also supposed to implement Object and Named.
please add a check for these 2 interfaces as well.
rootio/tchain.go
Outdated
| func (t tchain) Branches() []Branch { | ||
|
|
||
| return t.trees[0].Branches() | ||
| /*var branch []Branch |
There was a problem hiding this comment.
please remove this commented out snippet of code.
rootio/tchain.go
Outdated
| // TotBytes return the total number of bytes before compression. | ||
| func (t tchain) TotBytes() int64 { | ||
| var v int64 | ||
| for _,tree:= range t.trees { |
There was a problem hiding this comment.
this isn't gofmt'ed :)
please run:
$> gofmt -w .
in the rootio directory holding this file.
rootio/tchain.go
Outdated
| //Branches returns the list of branches. | ||
| func (t tchain) Branches() []Branch { | ||
| branch := t.trees[0].Branches() | ||
| if branch != nil { |
There was a problem hiding this comment.
as discussed offline, this isn't the proper check.
if len(t.trees) is 0, trying to access the first element of an empty slice will panic.
see, e.g.: https://play.golang.org/p/xbOu56NDsvq
panic: runtime error: index out of range
goroutine 1 [running]:
main.main()
/tmp/sandbox633982079/main.go:10 +0x80
the correct fix:
if len(t.trees) == 0 {
return nil
}
return t.trees[0].Branches()
rootio/tchain.go
Outdated
| //Branch returns the branch whose name is the argument. | ||
| func (t tchain) Branch(name string) Branch { | ||
| branch := t.trees[0].Branches() | ||
| if branch != nil { |
rootio/tchain.go
Outdated
| return nil | ||
| } | ||
|
|
||
| //Branch returns the branch whose name is the argument. |
There was a problem hiding this comment.
godoc is a bit picky about how to format the comments.
please leave a space between // and the start of the comment.
e.g.:
// Branch returns the branch whose name is the argument.
func (t tchain) Branch(name string) Branch {
sbinet
left a comment
sbinet
left a comment
There was a problem hiding this comment.
looking better :)
also: please add tests.
- one with a chain made of
chain1.rootandchain2.root - one with a chain made of a
nilslice of trees.
rootio/tchain.go
Outdated
| trees []Tree | ||
| } | ||
|
|
||
| //Class returns the ROOT class of the argument. |
There was a problem hiding this comment.
please add a space between // and the start of the comment.
rootio/tchain.go
Outdated
|
|
||
|
|
||
|
|
||
| //Name returns the name of the ROOT objet in the argument. |
There was a problem hiding this comment.
please add a space between // and the start of the comment.
rootio/tchain.go
Outdated
| } | ||
|
|
||
|
|
||
| //Title returns the title of the ROOT object in the argument. |
There was a problem hiding this comment.
please add a space between // and the start of the comment.
rootio/tchain.go
Outdated
| return t | ||
| } | ||
|
|
||
| //Entries returns the total number of entries |
There was a problem hiding this comment.
please add a space between // and the start of the comment.
rootio/tchain.go
Outdated
| return v | ||
| } | ||
|
|
||
| //ZipBytes returns the total number of bytes after compression. |
There was a problem hiding this comment.
please add a space between // and the start of the comment.
rootio/tchain.go
Outdated
| return t.trees[0].Leaves() | ||
| } | ||
|
|
||
| //getFile returns the underlying file. |
There was a problem hiding this comment.
please add a space between // and the start of the comment.
rootio/tchain.go
Outdated
| return t.trees[0].getFile() | ||
| } | ||
|
|
||
| //loadEntry returns an error if there is a problem during the loading |
There was a problem hiding this comment.
please add a space between // and the start of the comment.
| v += tree.ZipBytes() | ||
| } | ||
| return v | ||
|
|
rootio/tchain.go
Outdated
| if len(t.trees) == 0 { | ||
| return nil | ||
| } | ||
| for _, br := range t.trees[0].Branches() { |
There was a problem hiding this comment.
here, you could actually just forward to t.trees[0].Branch(name) and call it a day.
rootio/tchain.go
Outdated
| if len(t.trees) == 0 { | ||
| return nil | ||
| } | ||
| for _, b := range t.trees[0].Branches() { |
There was a problem hiding this comment.
here, you could actually just forward to t.trees[0].loadEntry(i) and call it a day.
|
the |
sbinet
left a comment
sbinet
left a comment
There was a problem hiding this comment.
one more comment (in addition to the gofmt issue, of course)
|
|
||
| // Chain returns a tchain that is the concatenation of all the input Trees. | ||
| func Chain(trees ...Tree) tchain { | ||
| var t tchain |
There was a problem hiding this comment.
thinking a bit more about this, perhaps it would be better (memory-wise) to:
- check for the length of the
treesslice and return a zero-value oftchainin case the length is 0 - create a
t.treesslice with directly the correct length and then use the builtincopyfunction
|
i added myself to the AUTHORS and CONTRIBUTORS files :) ( i hope that's how
it's done )
2018-04-16 16:54 GMT+02:00 Sebastien Binet <notifications@github.com>:
… ***@***.**** approved this pull request.
LGTM (Looks Good To Me)
Could you fork license <https://github.com/go-hep/license> and send
another PR (pull request) against that repository adding yourself to the
AUTHORS and CONTRIBUTORS files?
thanks. once that's done, I'll merge this pull request.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#162 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AkVUbqrar1jw0zPFPLYr8T53cO2n4qLSks5tpLC1gaJpZM4TRXOZ>
.
|
|
needs go-hep/license#6. |
|
closed via 4032944 |
2 participants
Fixes #44.