Add fraction accumulator (for calculating efficiencies, etc)

[gohil-jay]

Added standard efficiency accumulator with value(), variance() and confidence_interval()

Working here.

[HDembinski]

Hi @gohil-jay , great progress! I added new comments. I think the next step for you is to write some unit tests. The tests are located in the folder tests. We do the testing with the lightweight tests in Boost Core. Basic documentation is here: https://www.boost.org/doc/libs/1_79_0/libs/core/doc/html/core/lightweight_test.html.

Have a look how testing is done for other accumulators and try to make a minimal test for your efficiency accumulator. You could also start simpler and test your detail::waldInterval (that you should rename to detail::wald_interval`.

[HDembinski]

Most of the tests are failing, because I made a mistake, I am fixing that now.

[HDembinski]

Please merge the develop branch into this one to get the fix that I just commited. In the terminal, do git merge develop when the currently active checkout is your branch.

[gohil-jay]

This is what I see...

Are we trying to merge the developV1 branch into develop branch?

[HDembinski]

Edit: This has been fixed (temporarily?) by fixing the types of the helper functions to double, so can be regarded as resolved.

I didn't find a nice way of fixing the warning, so you have to use the ugly way. Something along these lines will silence the warning:

#include <array>
#include <boost/config/workaround.hpp>

int main() {

#if BOOST_WORKAROUND(BOOST_MSVC, >= 0)
#pragma warning(disable : 4305)
#pragma warning(disable : 4838)
#endif
  std::array<float, 2> x = {1.5, 2.1};
#if BOOST_WORKAROUND(BOOST_MSVC, >= 0)
#pragma warning(default : 4305)
#pragma warning(default : 4838)
#endif

  return 0;
}

[HDembinski]

Can you please do git checkout develop & git pull & git merge develop again? This will fix the issue you had with the DEBUG macro on Windows.

[gohil-jay]

Done!

[HDembinski]

Hi Jay, looks like all tests pass now, but you still need to fix the warning in detail_probit_test with the code in my earlier comment.

I have been thinking about the interface of this class, and we need to iterate it some more. Here are the requirements.

• The class should have minimal size, because we are potentially creating a lot of them. Minimal is two counters for successes and failures.
• We do not want to compute the conversion from CL to z every time someone is calling confidence_interval, because this is going to be called a lot and the conversion is expensive. We could pre-compute this when the class is created, but this is still expensive, since the code will be run for each accumulator instance that is created, and we potentially create a lot of them. Also, we then have to store the result of the computation in the class, and then the class is not of minimal size.
• Users are not going to switch the CL or the type of confidence interval dynamically, they will settle for one setting and then keep going with that. This means we can make these configurations part of the type, by using template parameters. Floating point numbers cannot be used as non-type template parameters, but we can use std::ratio instead https://stackoverflow.com/questions/2183087/why-cant-i-use-float-value-as-a-template-parameter.
• We should allow users to pass z directly without going through the CL to z conversion, because at least in physics we often express confidence levels in z sigma (number of standard deviations of a normal distribution). 5 sigma is easier to mentally parse than 0.9999994266968563.

I will play around with a few ideas on Godbolt. Once I have something, I will present it here. You should then let me know when you are done with your current changes so that I can integrate this part without us creating conflicts for each other by making changes simultaneously.

[HDembinski]

@gohil-jay After looking at the Clopper-Pearson interval, I think we cannot avoid to use Boost Math at least for this interval. I don't want to re-implement the regularised incomplete beta function in our detail namespace. But we can probably make it so that the user only has to have Boost Math available if they really want the Clopper-Pearson interval. I will investigate this. Furthermore, the beta distribution is need for the calculation of the Jeffreys interval.

[HDembinski]

@gohil-jay We need do some more work on this branch, but the "changes requested" warning is annoying me, so I accepted the changes you made so far.

[HDembinski]

I added two new classes which allow us to define the interval via confidence level or via a multiple of the standard deviation. In physics, this is common can now be easily supported.

[HDembinski]

I committed another large patch. The efficiency accumulator is now called fraction, since this is a more general name.

The fraction accumulator only offers the default interval, which is the Wilson score interval. Users can compute other intervals with the interval functors in the utility namespace which are publically exposed. I implemented the wilson_interval as an example for you, @gohil-jay , could you please implement the others? You will need special functions from Boost Math for clopper_pearson_interval and jeffreys_interval.

But first, please add some more tests to accumulators_fraction_test.cpp.

[HDembinski]

The new intervals start to look good but you started to make changes that broke things that were previously correct. I am talking especially about the changes to fraction and odr_test.cpp

[HDembinski]

@gohil-jay Can you work on fixing the failing tests? appveyor is complaining about possible loss of data

C:\projects\boost\boost/histogram/accumulators/fraction.hpp(35): error C2220: the following warning is treated as an error
test\accumulators_fraction_test.cpp(43): note: see reference to function template instantiation 'boost::histogram::accumulators::fraction<T>::fraction<double>(const boost::histogram::accumulators::fraction<double> &) noexcept' being compiled
        with
        [
            T=int
        ]
test\accumulators_fraction_test.cpp(43): note: see reference to function template instantiation 'boost::histogram::accumulators::fraction<T>::fraction<double>(const boost::histogram::accumulators::fraction<double> &) noexcept' being compiled
        with
        [
            T=int
        ]
test\accumulators_fraction_test.cpp(77): note: see reference to function template instantiation 'void run_tests<int>(void)' being compiled
C:\projects\boost\boost/histogram/accumulators/fraction.hpp(35): warning C4244: 'argument': conversion from 'double' to 'const int', possible loss of data
C:\projects\boost\boost/histogram/accumulators/fraction.hpp(35): warning C4244: 'argument': conversion from 'double' to 'const int', possible loss of data

[HDembinski]

Hi Jay, the comments that you added look ok. I am concerned about the test that you changed, and that we do not have an explanation for the large deviations of the jeffreys and clopper pearson intervals from the reference numbers. It is ok to have deviations of about 1e-7, but not 0.1.

[HDembinski]

The error in Slow/gcc10 is not caused by us, but in boostorg/math. I send a PR with a fix to boostorg/math.

[HDembinski]

@gohil-jay I fixed the jefferys_interval, you don't need to change this anymore. While reading up on it, I found a minor enhancement for this interval, which I implemented. It is all documented in the class.

While fixing the jeffreys_interval, I noticed why you were getting wrong results. You treated CL as alpha, but alpha = 1 - CL. This is also the reason why your implementation of the clopper_pearson interval gives wrong results. We can and should precompute alpha_half in the constructor of the interval class, because alpha_half is constant during the life-time of the interval maker. This is why we use a class, to be able to put computations that only need to be done once into the constructor. Please have a look how I did it in the jeffreys_interval class, and copy that for the clopper_pearson_interval.

[HDembinski]

I now also fixed the clopper-pearson interval. You don't need to change anything there anymore. Since the beta distribution from boost::math cannot handle the case where the successes or failures are zero, I use an explicit formula for that special case, which is described in the wikipedia.

[gohil-jay]

I will take a look at all the changes including one to boost::math to see how you overcame beta_distribution limit (maybe conditional implementation for those inputs or something I missed from docs) and rest of others.

[HDembinski]

I will take a look at all the changes including one to boost::math to see how you overcame beta_distribution limit (maybe conditional implementation for those inputs or something I missed from docs) and rest of others.

Yes, I special-cased this. For successes = 0 (or failures = 0), one can compute the interval boundaries analytically as described in the Wikipedia.

[HDembinski]

@henryiii Can you please look over the interface of accumulators/fraction.hpp and let us know if you see any issues? The interface to compute confidence_interval is intentionally not flexible, since people should use the wilson interval unless they really know what they are doing. We provide more ways to compute intervals with the interval computers in boost/histogram/utility.

[henryiii]

Is there an integration test somewhere (maybe I’m just missing it)? I see unit tests for pieces, but is there a test were a histogram with the new accumulator is tested? I wanted to see the bool sample being used, I’m thinking this is the first storage where a bool sample is expected. That's the most important usage from boost-histogram's perspective.

No docs yet?

To be sure the interface works in boost-histogram, we can always make a branch of boost-histogram building on this branch. But I don’t see anything that would be likely to be a problem.

[HDembinski]

@henryiii Thank you for the comments.

Is there an integration test somewhere (maybe I’m just missing it)? I see unit tests for pieces, but is there a test were a histogram with the new accumulator is tested? I wanted to see the bool sample being used, I’m thinking this is the first storage where a bool sample is expected. That's the most important usage from boost-histogram's perspective.

Good catch, I think we tested this in the beginning interactively on godbolt, but we don't have this integration test. Needs to be added.

No docs yet?

Do you mean the user guide? All new classes/functions are documented with doxygen strings. I also checked the generated docs, they look ok. But it is still missing from the guide, that's true.

To be sure the interface works in boost-histogram, we can always make a branch of boost-histogram building on this branch. But I don’t see anything that would be likely to be a problem.

Yes and until the next boost release, we can also make breaking changes.

[henryiii]

Do you mean the user guide?

Yes. Not at all a blocker, can be added later, just should be on a todo list somewhere.

[HDembinski]

There is indeed a problem with std::vector<bool>. Because of the specialization of vector, one cannot pass std::vector<bool> to histogram::fill since it is not compatible with std::span (or boost::span). It is possible with any contiguous containers of boolean values that does not use the vector specialization. Supporting std::vector<bool> in fill is difficult, I would have to create a special path just for that. What works is to use std::vector<char> instead of std::vector<bool>.

In boost-histogram, you want to pass a numpy array of bool, which in memory looks like std::vector<char>. Conversion to std::vector<bool> would actually invoke a costly copy. So supporting std::vector<bool> does not make much sense. This should be documented, of course.

[henryiii]

Looks great. Boost-histogram doesn't actually use std::vector (except for strings), so I think we'll be fine there.

[HDembinski]

Docs were improved, integration test added, and some convenience to allow the interval computers to accept a fraction accumulator directly. I will merge as soon as this passes all tests. Thanks @gohil-jay @henryiii