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

Change median and mean export, or update documentation #146

Closed
milesfrain opened this issue Sep 20, 2019 · 8 comments · Fixed by #336
Closed

Change median and mean export, or update documentation #146

milesfrain opened this issue Sep 20, 2019 · 8 comments · Fixed by #336

Comments

@milesfrain
Copy link
Contributor

milesfrain commented Sep 20, 2019

There's a mismatch between what the documentation suggests and how these functions are exported.

maximum and minimum are exported to Base, while median and mean are exported to Statistics.

Base.minimum(group::BenchmarkGroup) = mapvals(minimum, group)
Base.maximum(group::BenchmarkGroup) = mapvals(maximum, group)
Statistics.mean(group::BenchmarkGroup) = mapvals(mean, group)
Statistics.median(group::BenchmarkGroup) = mapvals(median, group)

The manual does not mention the need for using Statistics to get the the remaining two functions to work.

The reference guide also suggests that median and mean are exported the same as minimum and maximum.

Unsure whether median and mean should be exported to Base, since this wouldn't extending an existing Base method.

Similar to #143

@milesfrain milesfrain changed the title Export median and mean or update documentation Change median and mean export, or update documentation Sep 20, 2019
@ararslan
Copy link
Member

median and mean were removed from Base and put into Statistics. We should either reexport those or tell people to run using Statistics in the documentation.

@CameronBieganek
Copy link

I just bumped into this again. I had already forgotten that I needed to do using Statistics to get median and mean to work, and the BenchmarkTools documentation didn't remind me...

@ararslan
Copy link
Member

A PR to document or reexport mean and median would be welcome. I'm fairly indifferent on which is preferable, though I guess I have a slight inclination toward the latter.

@milesfrain
Copy link
Contributor Author

See #149 for the latter option

@gdalle
Copy link
Collaborator

gdalle commented Sep 18, 2023

Reopening to document these reexports

@gdalle gdalle added this to the v2.0 milestone Sep 18, 2023
@milesfrain
Copy link
Contributor Author

Is there additional documentation required? I thought the fix to re-export now ensures that the existing docs are now aligned with the code.

@gdalle
Copy link
Collaborator

gdalle commented Sep 24, 2023

Maybe just a remark stating that they're re exported is what I had in mind

@milesfrain
Copy link
Contributor Author

milesfrain commented Sep 25, 2023

Added the remark and applied some additional edits in #336

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants