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

Reduce space usage of Discrete Integrals #1877

Merged
merged 7 commits into from Feb 5, 2024
Merged

Conversation

yjian012
Copy link
Contributor

@yjian012 yjian012 commented Jan 6, 2024

Reduce space usage of discrete integrals, no need to allocate extra space.

@coveralls
Copy link

coveralls commented Jan 6, 2024

Coverage Status

coverage: 72.426% (+0.1%) from 72.31%
when pulling 19987be on yjian012:master
into dee33df on lballabio:master.

@pcaspers
Copy link
Contributor

pcaspers commented Jan 6, 2024

I get why you remove the array fv, but what is the benefit of removing the accumulators to compute a sum?

@yjian012
Copy link
Contributor Author

yjian012 commented Jan 6, 2024

I don't know the internal implementation of accumulators, but from what I read, it provides a bunch of statistics tools, like sum, moments, median, etc. To be able to calculate all these functions, it must store all the data, which requires dynamic memory allocation. But we only need one number, the sum.
So, unless there's some secret acceleration on summation that it can do, I think it should be more efficient without it.

@pcaspers
Copy link
Contributor

pcaspers commented Jan 6, 2024

Oh ok, but the sum accumulator does not store each summand, it only maintains the running sum as its internal state, see here:

https://github.com/boostorg/accumulators/blob/9d9e5dae2202660f57e2dc91efb620aa001525b3/include/boost/accumulators/statistics/sum.hpp#L43

@yjian012
Copy link
Contributor Author

yjian012 commented Jan 6, 2024

OK, I see. So when it's only tagged "sum", it only computes the sum... Thank you for letting me know. I still prefer a simple summation, since the area under the curve in each interval is not statistics variable, we'll never need any other feature of this class. But I don't really mind. I reverted it back to accumulator.

@sweemer
Copy link
Contributor

sweemer commented Jan 7, 2024

For what it's worth, I think that the simple sum is better because it is easier to understand, and also because it doesn't pull in all of the dependencies of boost accumulators, of which there are a lot. I understand that this is a cpp file and not an hpp file but it's good to reduce dependencies wherever we can.

@pcaspers
Copy link
Contributor

pcaspers commented Jan 7, 2024

Agreed. Maybe using std::accumulate would be preferable.

@lballabio lballabio added this to the Release 1.34 milestone Feb 5, 2024
@lballabio lballabio merged commit c6492d0 into lballabio:master Feb 5, 2024
48 checks passed
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.

None yet

5 participants