search

boostorg / histogram

Fast multi-dimensional generalized histogram with convenient interface for C++14
Boost Software License 1.0
313 stars 74 forks source link

Add support for weighted fraction accumulator #385

Open ryanelandt opened 1 year ago

ryanelandt commented 1 year ago

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 commented 1 year ago

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

ryanelandt commented 1 year ago

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

HDembinski commented 1 year ago

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 commented 1 year ago

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

HDembinski commented 1 year ago

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 commented 1 year ago

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.