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 collector accumulator #390

Closed wiso closed 2 months ago

wiso commented 8 months ago

This is a simple implementation of collector accumulator, a very simple accumulator which stores all the values passed, as suggested in #349. Some new tests are implemented. Documentation is updated.

I would like to have a code review. For sure these items are missing.

Probably implementing serialization for this is not a good idea.

I had to change the continuous integration:

wiso commented 3 months ago

is this project abandoned? @HDembinski

HDembinski commented 3 months ago

Not at all, but I have a lot of things on my plate right now. I should be able to work on all the outstanding issues/PRs in the coming months.

HDembinski commented 2 months ago

Please merge current develop branch to fix the CI issues.

HDembinski commented 2 months ago

You don't need to write a changelog message, that's why I removed that item from your todo list. Please add the Boost copyright notice to all new files.

wiso commented 2 months ago

what should I write? I see a common

//
// Distributed under the Boost Software License, version 1.0.
// (See accompanying file LICENSE_1_0.txt
// or copy at http://www.boost.org/LICENSE_1_0.txt)

What should be the first line?

HDembinski commented 2 months ago

It should be your name and my name. I am working on a large patch for collector, please don't make any other changes to the collector at the moment.

HDembinski commented 2 months ago

@henryiii The collector accumulator is finally moving forward and the implementation seems to be nearly ready. I would like to quickly see it in the boost-histogram Python package, too, since I want to use it in a Python study. What is your time budget right now, do you think you can look into this? Otherwise I can give it a try.

HDembinski commented 2 months ago

@wiso I think the PR is almost ready to be merged. I added the copyright notices.

Design changes and additions: I changed the template arguments for the collector, because your original interface was not DRY. I added missing serialization support. You were exposing the internal container, now the internal container remains hidden. Instead, I added basic methods to the collector, which make it iterable and allow element access, and give access to the memory buffer (this is needed by the Python boost-histogram wrapper).

Tests are in place for the interface. I have added a test which checks that the collector template can work with a dynamic array container type as outlined above.

Coverage is not 100% right now, but that will be fixed in future PRs. The tracking of line coverage in coveralls is a bit broken and I don't know yet how to fix this. I cannot bring coverage to 100 % if the tracking does not work correctly.

Do you wish to add anything at this point?

wiso commented 2 months ago

Hello @HDembinski , thank you very much for your improvement. I am not planning to add anything. I would be happy to see this included. About the Python package, I have not fully understood how it has been implemented, I thought it was a binding of boost-histogram, but I see several parts are rewritten. I don't have expertise on it, even if it is the main way I use hist.