[0/5] Fix histogram for some (corner) cases
mbox series

Message ID 20250324170803.103296-1-stefan.klug@ideasonboard.com
Headers show
Series
  • Fix histogram for some (corner) cases
Related show

Message

Stefan Klug March 24, 2025, 5:07 p.m. UTC 
Hi all,

during my work on WDR I stumbled over a few non-linearities when dealing
with small inter quantile mean ranges. These led to hard to handle
oscillations. This series adds a testcase for the histogram class and
fixes the related issues.

Patch 1 adds the basic tests that pass with the current implementation.

Patch 2-3 fix issues in the quantile() implementation and add
corresponding tests.

Patch 4-5 fix issues in the interQuantileMean() implementation and also
add tests.

I added the tests after the fixes, so that I don't have to toggle should_fail
in the meson.build file all the time (And CI doesn't test every commit anyways).
To test that in review, I believe the easiest is to temporarily revert the
corresponding fix and see the test fail.

Best regards,
Stefan

Stefan Klug (5):
  test: ipa: libipa: Add histogram tests
  libipa: histogram: Fix quantile() calculation for fractional results
  test: ipa: libipa: histogram: Add tests for quantile() returning a
    fraction
  libipa: histogram: Fix interQuantileMean() for small ranges
  test: ipa: libipa: histogram: Add tests for small inter quantile mean
    ranges

 src/ipa/libipa/histogram.cpp  | 23 ++++++------
 test/ipa/libipa/histogram.cpp | 66 +++++++++++++++++++++++++++++++++++
 test/ipa/libipa/meson.build   |  1 +
 3 files changed, 80 insertions(+), 10 deletions(-)
 create mode 100644 test/ipa/libipa/histogram.cpp

Comments

Laurent Pinchart March 31, 2025, 9:30 p.m. UTC | #1 
On Mon, Mar 24, 2025 at 06:07:35PM +0100, Stefan Klug wrote:
> Hi all,
> 
> during my work on WDR I stumbled over a few non-linearities when dealing
> with small inter quantile mean ranges. These led to hard to handle
> oscillations. This series adds a testcase for the histogram class and
> fixes the related issues.
> 
> Patch 1 adds the basic tests that pass with the current implementation.
> 
> Patch 2-3 fix issues in the quantile() implementation and add
> corresponding tests.
> 
> Patch 4-5 fix issues in the interQuantileMean() implementation and also
> add tests.
> 
> I added the tests after the fixes, so that I don't have to toggle should_fail
> in the meson.build file all the time (And CI doesn't test every commit anyways).

Doesn't it ? The build-history test is supposed to do just that.

> To test that in review, I believe the easiest is to temporarily revert the
> corresponding fix and see the test fail.

should_fail is meant exactly for this :-)

> Stefan Klug (5):
>   test: ipa: libipa: Add histogram tests
>   libipa: histogram: Fix quantile() calculation for fractional results
>   test: ipa: libipa: histogram: Add tests for quantile() returning a
>     fraction
>   libipa: histogram: Fix interQuantileMean() for small ranges
>   test: ipa: libipa: histogram: Add tests for small inter quantile mean
>     ranges
> 
>  src/ipa/libipa/histogram.cpp  | 23 ++++++------
>  test/ipa/libipa/histogram.cpp | 66 +++++++++++++++++++++++++++++++++++
>  test/ipa/libipa/meson.build   |  1 +
>  3 files changed, 80 insertions(+), 10 deletions(-)
>  create mode 100644 test/ipa/libipa/histogram.cpp
Laurent Pinchart March 31, 2025, 9:54 p.m. UTC | #2 
On Tue, Apr 01, 2025 at 12:30:04AM +0300, Laurent Pinchart wrote:
> On Mon, Mar 24, 2025 at 06:07:35PM +0100, Stefan Klug wrote:
> > Hi all,
> > 
> > during my work on WDR I stumbled over a few non-linearities when dealing
> > with small inter quantile mean ranges. These led to hard to handle
> > oscillations. This series adds a testcase for the histogram class and
> > fixes the related issues.
> > 
> > Patch 1 adds the basic tests that pass with the current implementation.
> > 
> > Patch 2-3 fix issues in the quantile() implementation and add
> > corresponding tests.
> > 
> > Patch 4-5 fix issues in the interQuantileMean() implementation and also
> > add tests.
> > 
> > I added the tests after the fixes, so that I don't have to toggle should_fail
> > in the meson.build file all the time (And CI doesn't test every commit anyways).
> 
> Doesn't it ? The build-history test is supposed to do just that.

Ah, you meant that CI doesn't run unit tests for every commit. That's
true.

> > To test that in review, I believe the easiest is to temporarily revert the
> > corresponding fix and see the test fail.
> 
> should_fail is meant exactly for this :-)

should_fail isn't meant for CI. Adding tests first ensures that the test
fails. This is meant to avoid adding a buggy test that doesn't actually
catch the issue it's supposed to catch. I would much prefer having the
tests added first.

> > Stefan Klug (5):
> >   test: ipa: libipa: Add histogram tests
> >   libipa: histogram: Fix quantile() calculation for fractional results
> >   test: ipa: libipa: histogram: Add tests for quantile() returning a
> >     fraction
> >   libipa: histogram: Fix interQuantileMean() for small ranges
> >   test: ipa: libipa: histogram: Add tests for small inter quantile mean
> >     ranges
> > 
> >  src/ipa/libipa/histogram.cpp  | 23 ++++++------
> >  test/ipa/libipa/histogram.cpp | 66 +++++++++++++++++++++++++++++++++++
> >  test/ipa/libipa/meson.build   |  1 +
> >  3 files changed, 80 insertions(+), 10 deletions(-)
> >  create mode 100644 test/ipa/libipa/histogram.cpp
Stefan Klug April 1, 2025, 1:07 p.m. UTC | #3 
On Tue, Apr 01, 2025 at 12:54:48AM +0300, Laurent Pinchart wrote:
> On Tue, Apr 01, 2025 at 12:30:04AM +0300, Laurent Pinchart wrote:
> > On Mon, Mar 24, 2025 at 06:07:35PM +0100, Stefan Klug wrote:
> > > Hi all,
> > > 
> > > during my work on WDR I stumbled over a few non-linearities when dealing
> > > with small inter quantile mean ranges. These led to hard to handle
> > > oscillations. This series adds a testcase for the histogram class and
> > > fixes the related issues.
> > > 
> > > Patch 1 adds the basic tests that pass with the current implementation.
> > > 
> > > Patch 2-3 fix issues in the quantile() implementation and add
> > > corresponding tests.
> > > 
> > > Patch 4-5 fix issues in the interQuantileMean() implementation and also
> > > add tests.
> > > 
> > > I added the tests after the fixes, so that I don't have to toggle should_fail
> > > in the meson.build file all the time (And CI doesn't test every commit anyways).
> > 
> > Doesn't it ? The build-history test is supposed to do just that.
> 
> Ah, you meant that CI doesn't run unit tests for every commit. That's
> true.
> 
> > > To test that in review, I believe the easiest is to temporarily revert the
> > > corresponding fix and see the test fail.
> > 
> > should_fail is meant exactly for this :-)
> 
> should_fail isn't meant for CI. Adding tests first ensures that the test
> fails. This is meant to avoid adding a buggy test that doesn't actually
> catch the issue it's supposed to catch. I would much prefer having the
> tests added first.

I somehow missed "meson test". That actually makes this quite useful and
now that I changed the order of patches I actually like it :-)

It would be great though to see all the tests that fail because the
check you mention above actually only works until the first failing
test. But that is for another day :-)

Regards,
Stefan

> 
> > > Stefan Klug (5):
> > >   test: ipa: libipa: Add histogram tests
> > >   libipa: histogram: Fix quantile() calculation for fractional results
> > >   test: ipa: libipa: histogram: Add tests for quantile() returning a
> > >     fraction
> > >   libipa: histogram: Fix interQuantileMean() for small ranges
> > >   test: ipa: libipa: histogram: Add tests for small inter quantile mean
> > >     ranges
> > > 
> > >  src/ipa/libipa/histogram.cpp  | 23 ++++++------
> > >  test/ipa/libipa/histogram.cpp | 66 +++++++++++++++++++++++++++++++++++
> > >  test/ipa/libipa/meson.build   |  1 +
> > >  3 files changed, 80 insertions(+), 10 deletions(-)
> > >  create mode 100644 test/ipa/libipa/histogram.cpp
> 
> -- 
> Regards,
> 
> Laurent Pinchart