Skip to content

Add option to specify median or mean instead of minimum time #41

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

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

chriselrod
Copy link
Collaborator

No description provided.

@chriselrod chriselrod merged commit dd3fbf2 into master Jan 28, 2021
@DilumAluthge DilumAluthge deleted the addsummarystats branch January 28, 2021 02:27
@DilumAluthge
Copy link
Member

summarystat = median

@chriselrod is this a typo? Did you mean minimum?

@chriselrod
Copy link
Collaborator Author

Yes, it should say minimum. I briefly considered making median the default because I think there should be some penalty for noise. But changing it may be considered breaking. And most users would expect the minimum to be the default, so I figured it's probably the better choice anyway.

I'll probably make another PR where I just record minimum, median, mean, and maximum, adding each to the benchmark_result. Then the plot function will let you choose which.

@chriselrod
Copy link
Collaborator Author

chriselrod commented Jan 28, 2021

Let's hold off on making a release until I do that. Removing an argument (summarystat from runbench is breaking, so let's just not make a release with it.

@DilumAluthge
Copy link
Member

Whooops I almost registered 😬 luckily I caught it in time 😂

JuliaRegistries/General#28820

@chriselrod
Copy link
Collaborator Author

Ha, that was close!

@DilumAluthge
Copy link
Member

Good thing we don't instant merge.

@DilumAluthge
Copy link
Member

FWIW, if it had been merged, we could have just yanked that version, so it wouldn't have been the end of the world. But better to not have merged in the first place 😉

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