| Message ID | 20261012110635.482661-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2026-10-12 12:06:35) > A few tests compare floating point values using the built-in equality > operators. This is not ideal at all, but bringing in nuance filled floating > point comparison does not appear too appealing either, especially just for a > couple tests. > > So instead work around these comparison failures by using numbers that are > exactly representable in base 2, and which ensure that the operations will be > exact as well. This is of course far from the best approach, but seems > acceptable given that the changes are very simple and only tests are affected > in the first place. > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/348 > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Seems like at least a clean workaround. I don't think it breaks any assumptions of what was being tested either so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > test/controls/control_value.cpp | 8 +++++--- > test/ipa/libipa/histogram.cpp | 6 +++--- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > index c2f9fc4826..399f4b2bb1 100644 > --- a/test/controls/control_value.cpp > +++ b/test/controls/control_value.cpp > @@ -272,19 +272,21 @@ protected: > /* > * Float type. > */ > - value.set(-0.42f); > + constexpr float fValue = -0.75; > + > + value.set(fValue); > if (value.isNone() || value.isArray() || > value.type() != ControlTypeFloat) { > cerr << "Control type mismatch after setting to float" << endl; > return TestFail; > } > > - if (value.get<float>() != -0.42f) { > + if (value.get<float>() != fValue) { > cerr << "Control value mismatch after setting to float" << endl; > return TestFail; > } > > - if (value.toString() != "-0.420000") { > + if (value.toString() != "-0.750000") { > cerr << "Control string mismatch after setting to float" << endl; > return TestFail; > } > diff --git a/test/ipa/libipa/histogram.cpp b/test/ipa/libipa/histogram.cpp > index 77ff31a6db..504c9ed319 100644 > --- a/test/ipa/libipa/histogram.cpp > +++ b/test/ipa/libipa/histogram.cpp > @@ -46,7 +46,7 @@ protected: > ASSERT_EQ(hist.quantile(0.75), 1.5); > > /* Test quantile smaller than the smallest histogram step. */ > - ASSERT_EQ(hist.quantile(0.001), 0.002); > + ASSERT_EQ(hist.quantile(0.125), 0.25); > > ASSERT_EQ(hist.interQuantileMean(0.0, 1.0), 1.0); > ASSERT_EQ(hist.interQuantileMean(0.0, 0.5), 0.5); > @@ -56,8 +56,8 @@ protected: > ASSERT_EQ(hist.interQuantileMean(0.25, 0.75), 1.0); > > /* Test small ranges at the borders of the histogram. */ > - ASSERT_EQ(hist.interQuantileMean(0.0, 0.1), 0.1); > - ASSERT_EQ(hist.interQuantileMean(0.9, 1.0), 1.9); > + ASSERT_EQ(hist.interQuantileMean(0.0, 0.125), 0.125); > + ASSERT_EQ(hist.interQuantileMean(0.875, 1.0), 1.875); > > return TestPass; > } > -- > 2.56.0 >
On Mon, Oct 12, 2026 at 12:21:02PM +0100, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2026-10-12 12:06:35) > > A few tests compare floating point values using the built-in equality > > operators. This is not ideal at all, but bringing in nuance filled floating > > point comparison does not appear too appealing either, especially just for a > > couple tests. > > > > So instead work around these comparison failures by using numbers that are > > exactly representable in base 2, and which ensure that the operations will be > > exact as well. This is of course far from the best approach, but seems > > acceptable given that the changes are very simple and only tests are affected > > in the first place. > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/348 > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Seems like at least a clean workaround. > > I don't think it breaks any assumptions of what was being tested either > so: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > test/controls/control_value.cpp | 8 +++++--- > > test/ipa/libipa/histogram.cpp | 6 +++--- > > 2 files changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > index c2f9fc4826..399f4b2bb1 100644 > > --- a/test/controls/control_value.cpp > > +++ b/test/controls/control_value.cpp > > @@ -272,19 +272,21 @@ protected: > > /* > > * Float type. > > */ > > - value.set(-0.42f); > > + constexpr float fValue = -0.75; I'm sad to see my 42 go, but that's for the best. I would have written kValue though (or maybe kfValue or kFloatValue ?), as we start constants with 'k'. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + > > + value.set(fValue); > > if (value.isNone() || value.isArray() || > > value.type() != ControlTypeFloat) { > > cerr << "Control type mismatch after setting to float" << endl; > > return TestFail; > > } > > > > - if (value.get<float>() != -0.42f) { > > + if (value.get<float>() != fValue) { > > cerr << "Control value mismatch after setting to float" << endl; > > return TestFail; > > } > > > > - if (value.toString() != "-0.420000") { > > + if (value.toString() != "-0.750000") { > > cerr << "Control string mismatch after setting to float" << endl; > > return TestFail; > > } > > diff --git a/test/ipa/libipa/histogram.cpp b/test/ipa/libipa/histogram.cpp > > index 77ff31a6db..504c9ed319 100644 > > --- a/test/ipa/libipa/histogram.cpp > > +++ b/test/ipa/libipa/histogram.cpp > > @@ -46,7 +46,7 @@ protected: > > ASSERT_EQ(hist.quantile(0.75), 1.5); > > > > /* Test quantile smaller than the smallest histogram step. */ > > - ASSERT_EQ(hist.quantile(0.001), 0.002); > > + ASSERT_EQ(hist.quantile(0.125), 0.25); > > > > ASSERT_EQ(hist.interQuantileMean(0.0, 1.0), 1.0); > > ASSERT_EQ(hist.interQuantileMean(0.0, 0.5), 0.5); > > @@ -56,8 +56,8 @@ protected: > > ASSERT_EQ(hist.interQuantileMean(0.25, 0.75), 1.0); > > > > /* Test small ranges at the borders of the histogram. */ > > - ASSERT_EQ(hist.interQuantileMean(0.0, 0.1), 0.1); > > - ASSERT_EQ(hist.interQuantileMean(0.9, 1.0), 1.9); > > + ASSERT_EQ(hist.interQuantileMean(0.0, 0.125), 0.125); > > + ASSERT_EQ(hist.interQuantileMean(0.875, 1.0), 1.875); > > > > return TestPass; > > }
Hi Barnabás s/x87/x86 in the patch title ? On Tue, Oct 13, 2026 at 01:58:13PM +0200, Laurent Pinchart wrote: > On Mon, Oct 12, 2026 at 12:21:02PM +0100, Kieran Bingham wrote: > > Quoting Barnabás Pőcze (2026-10-12 12:06:35) > > > A few tests compare floating point values using the built-in equality > > > operators. This is not ideal at all, but bringing in nuance filled floating > > > point comparison does not appear too appealing either, especially just for a > > > couple tests. > > > > > > So instead work around these comparison failures by using numbers that are > > > exactly representable in base 2, and which ensure that the operations will be > > > exact as well. This is of course far from the best approach, but seems > > > acceptable given that the changes are very simple and only tests are affected > > > in the first place. > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/348 > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > Seems like at least a clean workaround. > > > > I don't think it breaks any assumptions of what was being tested either > > so: > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > --- > > > test/controls/control_value.cpp | 8 +++++--- > > > test/ipa/libipa/histogram.cpp | 6 +++--- > > > 2 files changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > index c2f9fc4826..399f4b2bb1 100644 > > > --- a/test/controls/control_value.cpp > > > +++ b/test/controls/control_value.cpp > > > @@ -272,19 +272,21 @@ protected: > > > /* > > > * Float type. > > > */ > > > - value.set(-0.42f); > > > + constexpr float fValue = -0.75; > > I'm sad to see my 42 go, but that's for the best. I would have written > kValue though (or maybe kfValue or kFloatValue ?), as we start constants > with 'k'. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > + > > > + value.set(fValue); > > > if (value.isNone() || value.isArray() || > > > value.type() != ControlTypeFloat) { > > > cerr << "Control type mismatch after setting to float" << endl; > > > return TestFail; > > > } > > > > > > - if (value.get<float>() != -0.42f) { > > > + if (value.get<float>() != fValue) { > > > cerr << "Control value mismatch after setting to float" << endl; > > > return TestFail; > > > } > > > > > > - if (value.toString() != "-0.420000") { > > > + if (value.toString() != "-0.750000") { > > > cerr << "Control string mismatch after setting to float" << endl; > > > return TestFail; > > > } > > > diff --git a/test/ipa/libipa/histogram.cpp b/test/ipa/libipa/histogram.cpp > > > index 77ff31a6db..504c9ed319 100644 > > > --- a/test/ipa/libipa/histogram.cpp > > > +++ b/test/ipa/libipa/histogram.cpp > > > @@ -46,7 +46,7 @@ protected: > > > ASSERT_EQ(hist.quantile(0.75), 1.5); > > > > > > /* Test quantile smaller than the smallest histogram step. */ > > > - ASSERT_EQ(hist.quantile(0.001), 0.002); > > > + ASSERT_EQ(hist.quantile(0.125), 0.25); > > > > > > ASSERT_EQ(hist.interQuantileMean(0.0, 1.0), 1.0); > > > ASSERT_EQ(hist.interQuantileMean(0.0, 0.5), 0.5); > > > @@ -56,8 +56,8 @@ protected: > > > ASSERT_EQ(hist.interQuantileMean(0.25, 0.75), 1.0); > > > > > > /* Test small ranges at the borders of the histogram. */ > > > - ASSERT_EQ(hist.interQuantileMean(0.0, 0.1), 0.1); > > > - ASSERT_EQ(hist.interQuantileMean(0.9, 1.0), 1.9); > > > + ASSERT_EQ(hist.interQuantileMean(0.0, 0.125), 0.125); > > > + ASSERT_EQ(hist.interQuantileMean(0.875, 1.0), 1.875); > > > > > > return TestPass; > > > } > > -- > Regards, > > Laurent Pinchart
On Tue, Oct 13, 2026 at 02:22:57PM +0200, Jacopo Mondi wrote: > Hi Barnabás > > s/x87/x86 in the patch title ? "x87 is a floating-point-related subset of the x86 architecture instruction set." (https://en.wikipedia.org/wiki/X87) I assume that's what Barnabás meant. > On Tue, Oct 13, 2026 at 01:58:13PM +0200, Laurent Pinchart wrote: > > On Mon, Oct 12, 2026 at 12:21:02PM +0100, Kieran Bingham wrote: > > > Quoting Barnabás Pőcze (2026-10-12 12:06:35) > > > > A few tests compare floating point values using the built-in equality > > > > operators. This is not ideal at all, but bringing in nuance filled floating > > > > point comparison does not appear too appealing either, especially just for a > > > > couple tests. > > > > > > > > So instead work around these comparison failures by using numbers that are > > > > exactly representable in base 2, and which ensure that the operations will be > > > > exact as well. This is of course far from the best approach, but seems > > > > acceptable given that the changes are very simple and only tests are affected > > > > in the first place. > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/348 > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > Seems like at least a clean workaround. > > > > > > I don't think it breaks any assumptions of what was being tested either > > > so: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > --- > > > > test/controls/control_value.cpp | 8 +++++--- > > > > test/ipa/libipa/histogram.cpp | 6 +++--- > > > > 2 files changed, 8 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > > index c2f9fc4826..399f4b2bb1 100644 > > > > --- a/test/controls/control_value.cpp > > > > +++ b/test/controls/control_value.cpp > > > > @@ -272,19 +272,21 @@ protected: > > > > /* > > > > * Float type. > > > > */ > > > > - value.set(-0.42f); > > > > + constexpr float fValue = -0.75; > > > > I'm sad to see my 42 go, but that's for the best. I would have written > > kValue though (or maybe kfValue or kFloatValue ?), as we start constants > > with 'k'. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > + > > > > + value.set(fValue); > > > > if (value.isNone() || value.isArray() || > > > > value.type() != ControlTypeFloat) { > > > > cerr << "Control type mismatch after setting to float" << endl; > > > > return TestFail; > > > > } > > > > > > > > - if (value.get<float>() != -0.42f) { > > > > + if (value.get<float>() != fValue) { > > > > cerr << "Control value mismatch after setting to float" << endl; > > > > return TestFail; > > > > } > > > > > > > > - if (value.toString() != "-0.420000") { > > > > + if (value.toString() != "-0.750000") { > > > > cerr << "Control string mismatch after setting to float" << endl; > > > > return TestFail; > > > > } > > > > diff --git a/test/ipa/libipa/histogram.cpp b/test/ipa/libipa/histogram.cpp > > > > index 77ff31a6db..504c9ed319 100644 > > > > --- a/test/ipa/libipa/histogram.cpp > > > > +++ b/test/ipa/libipa/histogram.cpp > > > > @@ -46,7 +46,7 @@ protected: > > > > ASSERT_EQ(hist.quantile(0.75), 1.5); > > > > > > > > /* Test quantile smaller than the smallest histogram step. */ > > > > - ASSERT_EQ(hist.quantile(0.001), 0.002); > > > > + ASSERT_EQ(hist.quantile(0.125), 0.25); > > > > > > > > ASSERT_EQ(hist.interQuantileMean(0.0, 1.0), 1.0); > > > > ASSERT_EQ(hist.interQuantileMean(0.0, 0.5), 0.5); > > > > @@ -56,8 +56,8 @@ protected: > > > > ASSERT_EQ(hist.interQuantileMean(0.25, 0.75), 1.0); > > > > > > > > /* Test small ranges at the borders of the histogram. */ > > > > - ASSERT_EQ(hist.interQuantileMean(0.0, 0.1), 0.1); > > > > - ASSERT_EQ(hist.interQuantileMean(0.9, 1.0), 1.9); > > > > + ASSERT_EQ(hist.interQuantileMean(0.0, 0.125), 0.125); > > > > + ASSERT_EQ(hist.interQuantileMean(0.875, 1.0), 1.875); > > > > > > > > return TestPass; > > > > }
On Tue, Oct 13, 2026 at 02:41:45PM +0200, Laurent Pinchart wrote: > On Tue, Oct 13, 2026 at 02:22:57PM +0200, Jacopo Mondi wrote: > > Hi Barnabás > > > > s/x87/x86 in the patch title ? > > "x87 is a floating-point-related subset of the x86 architecture > instruction set." (https://en.wikipedia.org/wiki/X87) > > I assume that's what Barnabás meant. Ah indeed, I thought it was a type being the issue about x86 ... > > > On Tue, Oct 13, 2026 at 01:58:13PM +0200, Laurent Pinchart wrote: > > > On Mon, Oct 12, 2026 at 12:21:02PM +0100, Kieran Bingham wrote: > > > > Quoting Barnabás Pőcze (2026-10-12 12:06:35) > > > > > A few tests compare floating point values using the built-in equality > > > > > operators. This is not ideal at all, but bringing in nuance filled floating > > > > > point comparison does not appear too appealing either, especially just for a > > > > > couple tests. > > > > > > > > > > So instead work around these comparison failures by using numbers that are > > > > > exactly representable in base 2, and which ensure that the operations will be > > > > > exact as well. This is of course far from the best approach, but seems > > > > > acceptable given that the changes are very simple and only tests are affected > > > > > in the first place. > > > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/348 > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > > > Seems like at least a clean workaround. > > > > > > > > I don't think it breaks any assumptions of what was being tested either > > > > so: > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > --- > > > > > test/controls/control_value.cpp | 8 +++++--- > > > > > test/ipa/libipa/histogram.cpp | 6 +++--- > > > > > 2 files changed, 8 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > > > index c2f9fc4826..399f4b2bb1 100644 > > > > > --- a/test/controls/control_value.cpp > > > > > +++ b/test/controls/control_value.cpp > > > > > @@ -272,19 +272,21 @@ protected: > > > > > /* > > > > > * Float type. > > > > > */ > > > > > - value.set(-0.42f); > > > > > + constexpr float fValue = -0.75; > > > > > > I'm sad to see my 42 go, but that's for the best. I would have written > > > kValue though (or maybe kfValue or kFloatValue ?), as we start constants > > > with 'k'. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > + > > > > > + value.set(fValue); > > > > > if (value.isNone() || value.isArray() || > > > > > value.type() != ControlTypeFloat) { > > > > > cerr << "Control type mismatch after setting to float" << endl; > > > > > return TestFail; > > > > > } > > > > > > > > > > - if (value.get<float>() != -0.42f) { > > > > > + if (value.get<float>() != fValue) { > > > > > cerr << "Control value mismatch after setting to float" << endl; > > > > > return TestFail; > > > > > } > > > > > > > > > > - if (value.toString() != "-0.420000") { > > > > > + if (value.toString() != "-0.750000") { > > > > > cerr << "Control string mismatch after setting to float" << endl; > > > > > return TestFail; > > > > > } > > > > > diff --git a/test/ipa/libipa/histogram.cpp b/test/ipa/libipa/histogram.cpp > > > > > index 77ff31a6db..504c9ed319 100644 > > > > > --- a/test/ipa/libipa/histogram.cpp > > > > > +++ b/test/ipa/libipa/histogram.cpp > > > > > @@ -46,7 +46,7 @@ protected: > > > > > ASSERT_EQ(hist.quantile(0.75), 1.5); > > > > > > > > > > /* Test quantile smaller than the smallest histogram step. */ > > > > > - ASSERT_EQ(hist.quantile(0.001), 0.002); > > > > > + ASSERT_EQ(hist.quantile(0.125), 0.25); > > > > > > > > > > ASSERT_EQ(hist.interQuantileMean(0.0, 1.0), 1.0); > > > > > ASSERT_EQ(hist.interQuantileMean(0.0, 0.5), 0.5); > > > > > @@ -56,8 +56,8 @@ protected: > > > > > ASSERT_EQ(hist.interQuantileMean(0.25, 0.75), 1.0); > > > > > > > > > > /* Test small ranges at the borders of the histogram. */ > > > > > - ASSERT_EQ(hist.interQuantileMean(0.0, 0.1), 0.1); > > > > > - ASSERT_EQ(hist.interQuantileMean(0.9, 1.0), 1.9); > > > > > + ASSERT_EQ(hist.interQuantileMean(0.0, 0.125), 0.125); > > > > > + ASSERT_EQ(hist.interQuantileMean(0.875, 1.0), 1.875); > > > > > > > > > > return TestPass; > > > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp index c2f9fc4826..399f4b2bb1 100644 --- a/test/controls/control_value.cpp +++ b/test/controls/control_value.cpp @@ -272,19 +272,21 @@ protected: /* * Float type. */ - value.set(-0.42f); + constexpr float fValue = -0.75; + + value.set(fValue); if (value.isNone() || value.isArray() || value.type() != ControlTypeFloat) { cerr << "Control type mismatch after setting to float" << endl; return TestFail; } - if (value.get<float>() != -0.42f) { + if (value.get<float>() != fValue) { cerr << "Control value mismatch after setting to float" << endl; return TestFail; } - if (value.toString() != "-0.420000") { + if (value.toString() != "-0.750000") { cerr << "Control string mismatch after setting to float" << endl; return TestFail; } diff --git a/test/ipa/libipa/histogram.cpp b/test/ipa/libipa/histogram.cpp index 77ff31a6db..504c9ed319 100644 --- a/test/ipa/libipa/histogram.cpp +++ b/test/ipa/libipa/histogram.cpp @@ -46,7 +46,7 @@ protected: ASSERT_EQ(hist.quantile(0.75), 1.5); /* Test quantile smaller than the smallest histogram step. */ - ASSERT_EQ(hist.quantile(0.001), 0.002); + ASSERT_EQ(hist.quantile(0.125), 0.25); ASSERT_EQ(hist.interQuantileMean(0.0, 1.0), 1.0); ASSERT_EQ(hist.interQuantileMean(0.0, 0.5), 0.5); @@ -56,8 +56,8 @@ protected: ASSERT_EQ(hist.interQuantileMean(0.25, 0.75), 1.0); /* Test small ranges at the borders of the histogram. */ - ASSERT_EQ(hist.interQuantileMean(0.0, 0.1), 0.1); - ASSERT_EQ(hist.interQuantileMean(0.9, 1.0), 1.9); + ASSERT_EQ(hist.interQuantileMean(0.0, 0.125), 0.125); + ASSERT_EQ(hist.interQuantileMean(0.875, 1.0), 1.875); return TestPass; }
A few tests compare floating point values using the built-in equality operators. This is not ideal at all, but bringing in nuance filled floating point comparison does not appear too appealing either, especially just for a couple tests. So instead work around these comparison failures by using numbers that are exactly representable in base 2, and which ensure that the operations will be exact as well. This is of course far from the best approach, but seems acceptable given that the changes are very simple and only tests are affected in the first place. Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/348 Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- test/controls/control_value.cpp | 8 +++++--- test/ipa/libipa/histogram.cpp | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-)