Observe multiple from generic input iterators

[sjanel]

Addition of an observe method from generic input iterator, with a new unit test.

[sjanel]

Hi, no thoughts about this PR? Thanks

[waldheinz]

Not that my opinion matters here but I had a look anyway, and I like it. :smile:

Still, I think this would be even better if

• there were multiple commits and
• in the ObserveMultiple method I'd probably like to have the bucket count check upfront, using std::distance(from, end) != bucket_counts_.size(). This way the check is not duplicated and moved out of the loop. Also, we prevent messing up the internal state (in case the "late" check finds that this was in fact invalid input).

[sjanel]

Not that my opinion matters here but I had a look anyway, and I like it. smile

Still, I think this would be even better if

• there were multiple commits and
• in the ObserveMultiple method I'd probably like to have the bucket count check upfront, using std::distance(from, end) != bucket_counts_.size(). This way the check is not duplicated and moved out of the loop. Also, we prevent messing up the internal state (in case the "late" check finds that this was in fact invalid input).

Thanks for your remarks! OK for the split of commits. For the second point, for generic code it's better to iterate only once on the elements as if the iterator is not random access type (for instance, forward iterator only), it may:

• introduce performance degradation for non random access iterators, but, even worse:
• have unexpected side effects (or even maybe not possible for some complex iterators) if elements are looped twice.

To leverage above issues coming with generic iterators, I may specialize the function for random access iterators with your proposal. WDYT?

[gjasny]

Hello, would you please split your PR into smaller pieces? That eases review and discussion. Thanks, Gregor

[sjanel]

Hi,

OK, here is the first extracted PR with move constructors.

[waldheinz]

To leverage above issues coming with generic iterators, I may specialize the function for random access iterators with your proposal. WDYT?

The use case I have in mind for the extended API would use an std::array or maybe a std::vector to feed the new method, where an iterator is basically a pointer and std::distance boils down to a subtraction. For those cases doing the check upfront (or maybe even not at all; just mention it's UB in the documentation) would be preferable.

I see there are situations where it's not so clear and a single forward iteration would be preferable. But then I still think the method should either throw an exception on illegal arguments or succeed completely. The proposed implementation would throw an exception after the "damage" has already been done, when some samples have already been merged into the histogram. I consider this problematic. But OTOH I really don't know what to do with this kind of iterators. Maybe it's better to use the old API in this case?