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

Add support for weighted fraction accumulator #385

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

ryanelandt
Copy link

@ryanelandt ryanelandt commented Jun 5, 2023

This PR extends fractionto weighted samples to address issue #367.

I took the approach of creating a new weighted_fraction class. This class is a composition of two classes: fraction and a new internal class sum_of_weights_squared. I explain the thought process behind these choices.

The classes sum, mean, and fraction are non-weighted. Each of these classes could almost function as a weighted class, but requires one additional piece of information: the sum of the weights squared, that is to say $\sum (w^2)$. So, I turned this piece of information into a class sum_of_weights_squared. I created the class weighted_fraction by putting fraction and sum_of_weights_squared together.

The code passes all unit tests (locally), but has Wpedantic warnings because wilson_solve uses a designated initializer (i.e., wilson_solve({.n_eff=n_eff, .p_hat=p_hat, .correction=correction})). There's likely a good way to avoid getting this warning, while also enforcing input meaning, but I'm missing it.

@HDembinski
Copy link
Collaborator

Thanks for this substantial contribution, I will review it next weekend. I think you can replace the initializer to remove the warning.

@@ -36,7 +37,8 @@ class fraction {
using const_reference = const value_type&;
using real_type = typename std::conditional<std::is_floating_point<value_type>::value,
value_type, double>::type;
using interval_type = typename utility::wilson_interval<real_type>::interval_type;
using score_type = typename utility::wilson_interval<real_type>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

score_type should not appear here, because it is not a type that needs to be publicly visible.

Copy link
Author

Choose a reason for hiding this comment

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

This issue should be addressed now. Please take a look.

@ryanelandt
Copy link
Author

The failing check is due to the job never starting and eventually timing out.

@HDembinski
Copy link
Collaborator

This sometimes happens. I restarted the job. If it continues to fail, we can ignore it. Checking compatibility with an ancient compiler becomes less and less useful as time progresses.

@ryanelandt
Copy link
Author

Please let me know if there are any changes I can make to this.

@HDembinski
Copy link
Collaborator

I apologize for being slow with responding. I appreciate your enthusiasm to contribute to Boost.Histogram. I need some time to carefully think about the APIs, because in Boost we have very high standards in regards to stability.

It would be a good idea to start a new subfolder called experimental which contains new components such as this. Components in experimental are not promiosed to have a stable API. This would lower the bar for adoption.

@ryanelandt
Copy link
Author

I moved the weighted_fraction class to an experimental folder. I also made weighted_fraction a friend of fraction which helps minimize changes to the public API. Let me know if there are any other changes I can make.

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