search

boostorg / histogram

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

ASCII output for 1D histograms #201

Closed przemb closed 4 years ago

przemb commented 5 years ago

Hi, I have prepared an initial version of ascii visualization for 1d histograms. I still need to add a few things and refine tests, this PR is only to get early-stage feedback.

HDembinski commented 5 years ago

Thank you for the first draft of the implementation. I wrote you a first review. You need to redo it from scratch, sorry, but unfortunately that's how it works when you want to play in the Boost league. I had to rewrite this library three times from scratch as well until it was in the current state.

One thing you do a lot and which I also used to do in the past is to first collect all the information you need into your own data structures and then work on those. That's nice for you, because you decouple your implementation from the surrounding implementation, but you do it at a high cost, a lot of dynamic memory allocations. Allocating memory from the heap costs 1000s of CPU cycles, meaning you could do 1000s of computations in the meantime for each allocation. Performance is usually not that important for printing to the terminal, but you don't know how the library is going to be used, so you need to prepare for scenarios where people want to avoid dynamic allocations.

Boost.Histogram supports such environments. You can generate histograms completely on the stack. This is a standard that should be maintained.

You can still decouple your implementation from the surrounding code with transforming iterators. A transforming iterator walks over the original data structure but when you dereference it, it turns a transformed result. See https://www.boost.org/doc/libs/1_66_0/libs/iterator/doc/transform_iterator.html to understand what I mean. Unfortunately, you may not use Boost.Iterator, because we want the library to be decoupled, so you have to write your own transforming iterators.

HDembinski commented 5 years ago

Once the basic implementation stands, you should also add some unit tests.

przemb commented 5 years ago

Hi, thank you for all comments. Soon I will rewrite the whole display() function. I have just added tests for the existing implementation (based on serialization tests). Of course, I will modify them when I introduce missing ostream operator.

HDembinski commented 5 years ago

Hi, let me know when I should have another look

przemb commented 5 years ago

Hi, I am ready for the second review.

HDembinski commented 5 years ago

It is difficult to find performance guarantees for std::function, but for boost::function (precursor) they are listed https://www.boost.org/doc/libs/1_71_0/doc/html/function/misc.html

HDembinski commented 5 years ago

Since the printing for simple 1D histograms is close to final, we also need to think about how this change affects the printing for histograms which are not 1D. If the new display cannot handle the histogram, it should fall back to the old-style streaming.

HDembinski commented 4 years ago

Excellent and thank you very much for your contribution :) I will work on extending this for N dimensions in another branch.

mloskot commented 4 years ago

Awesome work! Cross-referencing the related issue #74