| Message ID | 20260810103846.1075936-36-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Quoting Barnabás Pőcze (2026-08-10 12:38:31) > Add an `operator[]` that takes an integral bin number and returns > the count pertaining to that specific bin. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/libipa/histogram.cpp | 6 ++++++ > src/ipa/libipa/histogram.h | 7 +++++++ > 2 files changed, 13 insertions(+) > > diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp > index bcf2639082..2f91ffb6d7 100644 > --- a/src/ipa/libipa/histogram.cpp > +++ b/src/ipa/libipa/histogram.cpp > @@ -74,6 +74,12 @@ Histogram::Histogram(Span<const uint32_t> data) > * \return Number of values > */ > > +/** > + * \fn Histogram::operator[](size_t bin) const > + * \brief Retrieve the value for \a bin > + * \return The value in \a bin, or 0 if \a bin is out of range I think that we can crash if bin is out of range. Imho that would be in line with other areas where we expect the caller to provide sensible values. Or am I missing a use case? > + */ > + > /** > * \brief Cumulative frequency up to a (fractional) point in a bin > * \param[in] bin The bin up to which to cumulate > diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h > index 0c02dda2ba..b3f59b7c31 100644 > --- a/src/ipa/libipa/histogram.h > +++ b/src/ipa/libipa/histogram.h > @@ -42,6 +42,13 @@ public: > double quantile(double q, uint32_t first = 0, uint32_t last = UINT_MAX) const; > double interQuantileMean(double lowQuantile, double hiQuantile) const; > > + uint32_t operator[](size_t bin) const > + { > + return bin < bins() > + ? cumulative_[bin + 1] - cumulative_[bin] > + : 0; I'd drop the bin < bins() check. Best regards, Stefan > + } > + > private: > std::vector<uint64_t> cumulative_; > }; > -- > 2.55.0 >
2026. 08. 12. 11:54 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Quoting Barnabás Pőcze (2026-08-10 12:38:31) >> Add an `operator[]` that takes an integral bin number and returns >> the count pertaining to that specific bin. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> --- >> src/ipa/libipa/histogram.cpp | 6 ++++++ >> src/ipa/libipa/histogram.h | 7 +++++++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp >> index bcf2639082..2f91ffb6d7 100644 >> --- a/src/ipa/libipa/histogram.cpp >> +++ b/src/ipa/libipa/histogram.cpp >> @@ -74,6 +74,12 @@ Histogram::Histogram(Span<const uint32_t> data) >> * \return Number of values >> */ >> >> +/** >> + * \fn Histogram::operator[](size_t bin) const >> + * \brief Retrieve the value for \a bin >> + * \return The value in \a bin, or 0 if \a bin is out of range > > I think that we can crash if bin is out of range. Imho that would be in > line with other areas where we expect the caller to provide sensible > values. Or am I missing a use case? Later changes do not depend on this behaviour. I think I went with this to be consistent with `cumulativeFrequency()`, which does handle an out of range `bin` parameter. > >> + */ >> + >> /** >> * \brief Cumulative frequency up to a (fractional) point in a bin >> * \param[in] bin The bin up to which to cumulate >> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h >> index 0c02dda2ba..b3f59b7c31 100644 >> --- a/src/ipa/libipa/histogram.h >> +++ b/src/ipa/libipa/histogram.h >> @@ -42,6 +42,13 @@ public: >> double quantile(double q, uint32_t first = 0, uint32_t last = UINT_MAX) const; >> double interQuantileMean(double lowQuantile, double hiQuantile) const; >> >> + uint32_t operator[](size_t bin) const >> + { >> + return bin < bins() >> + ? cumulative_[bin + 1] - cumulative_[bin] >> + : 0; > > I'd drop the bin < bins() check. > > Best regards, > Stefan > >> + } >> + >> private: >> std::vector<uint64_t> cumulative_; >> }; >> -- >> 2.55.0 >>
Quoting Barnabás Pőcze (2026-08-12 19:41:45) > 2026. 08. 12. 11:54 keltezéssel, Stefan Klug írta: > > Hi Barnabás, > > > > Quoting Barnabás Pőcze (2026-08-10 12:38:31) > >> Add an `operator[]` that takes an integral bin number and returns > >> the count pertaining to that specific bin. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> --- > >> src/ipa/libipa/histogram.cpp | 6 ++++++ > >> src/ipa/libipa/histogram.h | 7 +++++++ > >> 2 files changed, 13 insertions(+) > >> > >> diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp > >> index bcf2639082..2f91ffb6d7 100644 > >> --- a/src/ipa/libipa/histogram.cpp > >> +++ b/src/ipa/libipa/histogram.cpp > >> @@ -74,6 +74,12 @@ Histogram::Histogram(Span<const uint32_t> data) > >> * \return Number of values > >> */ > >> > >> +/** > >> + * \fn Histogram::operator[](size_t bin) const > >> + * \brief Retrieve the value for \a bin > >> + * \return The value in \a bin, or 0 if \a bin is out of range > > > > I think that we can crash if bin is out of range. Imho that would be in > > line with other areas where we expect the caller to provide sensible > > values. Or am I missing a use case? > > Later changes do not depend on this behaviour. I think I went with this to be consistent > with `cumulativeFrequency()`, which does handle an out of range `bin` parameter. > Set aside that cumulativeFrequency() is never used :-), the case is still a bit different. cumulativeFrequency() is passed a double value. So it makes sense (and is nice to the caller) to guard against illegal values that could be caused by some floating point/rounding error in the calling code. This function gets passed an integer. So I'd still drop it. Best regards, Stefan > > > > >> + */ > >> + > >> /** > >> * \brief Cumulative frequency up to a (fractional) point in a bin > >> * \param[in] bin The bin up to which to cumulate > >> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h > >> index 0c02dda2ba..b3f59b7c31 100644 > >> --- a/src/ipa/libipa/histogram.h > >> +++ b/src/ipa/libipa/histogram.h > >> @@ -42,6 +42,13 @@ public: > >> double quantile(double q, uint32_t first = 0, uint32_t last = UINT_MAX) const; > >> double interQuantileMean(double lowQuantile, double hiQuantile) const; > >> > >> + uint32_t operator[](size_t bin) const > >> + { > >> + return bin < bins() > >> + ? cumulative_[bin + 1] - cumulative_[bin] > >> + : 0; > > > > I'd drop the bin < bins() check. > > > > Best regards, > > Stefan > > > >> + } > >> + > >> private: > >> std::vector<uint64_t> cumulative_; > >> }; > >> -- > >> 2.55.0 > >> >
diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp index bcf2639082..2f91ffb6d7 100644 --- a/src/ipa/libipa/histogram.cpp +++ b/src/ipa/libipa/histogram.cpp @@ -74,6 +74,12 @@ Histogram::Histogram(Span<const uint32_t> data) * \return Number of values */ +/** + * \fn Histogram::operator[](size_t bin) const + * \brief Retrieve the value for \a bin + * \return The value in \a bin, or 0 if \a bin is out of range + */ + /** * \brief Cumulative frequency up to a (fractional) point in a bin * \param[in] bin The bin up to which to cumulate diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h index 0c02dda2ba..b3f59b7c31 100644 --- a/src/ipa/libipa/histogram.h +++ b/src/ipa/libipa/histogram.h @@ -42,6 +42,13 @@ public: double quantile(double q, uint32_t first = 0, uint32_t last = UINT_MAX) const; double interQuantileMean(double lowQuantile, double hiQuantile) const; + uint32_t operator[](size_t bin) const + { + return bin < bins() + ? cumulative_[bin + 1] - cumulative_[bin] + : 0; + } + private: std::vector<uint64_t> cumulative_; };