-
Notifications
You must be signed in to change notification settings - Fork 33
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
Compute confidence intervals in DistributedTreeDriver #83
Compute confidence intervals in DistributedTreeDriver #83
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need more time to review the math but here are a few questions/comments:
- I would probably want to merge Improve benchmark for distributed tree #78 first and rebase this onto master
- As much as I like the idea of computing confidence intervals and your using Boost.Accumulators I feel like the way
TimeMonitor
changes here is quite a stretch and it makes me question some of the design choices. I am more tempted to keep responsibilities separated: (i) a utility that measure time in a distributed context and (ii) a tool for statistics. (Please note that I do not advocate for the current design ofTimeMonitor
but I am even more skeptic about its evolution)
a9a88c5
to
aa21dc0
Compare
I have no idea why compiling with |
double const current_mean = ba::mean(_statistics); | ||
auto const tmp = | ||
z * current_stddev / (relative_error_margin * current_mean / 2.); | ||
return static_cast<int>(std::ceil(tmp * tmp)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I couldn't find parameters for boost::math::students_t::find_degrees_of_freedom
such that the returned value would match the one computed here.
boost::math::students_t::find_degrees_of_freedom(relative_error_margin*current_mean, confidence/2, .9999999, current_stddev)
is just 1.3 times larger though.
Just having a
member variable gives
when compiling with |
86fbf57
to
8f9bc84
Compare
Anyone opposed to closing this? It was a nice attempt, but too complicated with little benefit in practice. |
Yes, I think we won't do that. |
Based on #78, this pull requests improves
DistributedTreeDriver
further. The idea is as follows:construction
,knn
,radius
). (We take the maximum over all MPI processes in each iteration).All this is based on
CppCon 2015: Bryce Adelstein-Lelbach “Benchmarking C++ Code
andBoost.Accumulators
.Sample output: