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

WIP: Benchmark flatten #234

Merged
merged 1 commit into from
Oct 31, 2018
Merged

WIP: Benchmark flatten #234

merged 1 commit into from
Oct 31, 2018

Conversation

mschauer
Copy link
Contributor

@mschauer mschauer commented Oct 24, 2018

I think I do not understand the concept of addgroup! correctly, help welcome. In any case, these Benchmarks would be useful with JuliaLang/julia#29786 in mind.

@KristofferC KristofferC merged commit d77f701 into JuliaCI:master Oct 31, 2018
Copy link
Member

@jrevels jrevels left a comment

Choose a reason for hiding this comment

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

This PR deletes an existing BenchmarkGroup accidentally, other than that LGTM

###############################################
# flatten iterator

g = addgroup!(SUITE, "iterators", ["flatten"])
Copy link
Member

@jrevels jrevels Oct 31, 2018

Choose a reason for hiding this comment

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

This overwrites the other "iterators" group defined above, which was likely not your intent.

Instead, it would probably make sense just to add these benchmarks to this existing "iterators" group (e.g. just delete g = addgroup!(SUITE, "iterators", ["flatten"]), add "flatten" as a tag to the existing group, and remove the comment separator).

See also: https://github.com/JuliaCI/BenchmarkTools.jl/blob/master/doc/reference.md#addgroupsuitebenchmarkgroup-id-args

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that.

Would it make sense for addgroup! to error in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, I tend to think that would be a good idea ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for addgroup! to error in this case?

Yeah, this would probably be more useful than the current behavior, which is for addgroup!(suite, id, args...) to be a convenience function implementing g = BenchmarkGroup(args...); suite[id] = g; g.

KristofferC added a commit that referenced this pull request Oct 31, 2018
KristofferC added a commit that referenced this pull request Oct 31, 2018
@KristofferC
Copy link
Contributor

KristofferC commented Oct 31, 2018

@mschauer I reverted this based on the comments by Jarrett.

@mschauer mschauer mentioned this pull request Nov 1, 2018
@mschauer
Copy link
Contributor Author

mschauer commented Nov 1, 2018

Thank you for the help, fixed and rebased at #237

Keno pushed a commit that referenced this pull request Feb 4, 2022
* Show trials with 1 sample without error

When only a single sample is present, show the info on that one result.
Also use plurals appropriately with sample(s) and evaluation(s).

* Add test for displaying single-sample trial
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