[v2] test: ipa: rkisp1: utils: Fix floating and fixed point conversion test

20243 diff mbox series

There was an issue where using map to store the test cases meant that
the test for ignoring unused bits was skipped because of clashing keys.
Fix this by moving the offending test out of the loop.

While at it, also change the arbitrary floating comparison precision to
be more precise.

Also fix a missing documentation brief.

Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to
fixed-point
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

---
Changes in v2:
- remove the added complexity that would've made it clearner to add a
  larger variety of tests because the variety wasn't there
---
 test/ipa/rkisp1/rkisp1-utils.cpp | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Quoting Paul Elder (2024-06-07 11:03:23)
> There was an issue where using map to store the test cases meant that
> the test for ignoring unused bits was skipped because of clashing keys.
> Fix this by moving the offending test out of the loop.
> 
> While at it, also change the arbitrary floating comparison precision to
> be more precise.
> 
> Also fix a missing documentation brief.
> 
> Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to
> fixed-point

I think it's better to keep trailier lines on a single line even if it
extends beyond 72/80 chars.

I think the title is normally in brackets and quotes too?

Fixes: 9d152e9c6 ("ipa: rkisp1: Add a helper to convert floating-point to fixed-point")

But just trivial nits. That doesn't seem to be enforced by checkstyle
presently.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> Changes in v2:
> - remove the added complexity that would've made it clearner to add a
>   larger variety of tests because the variety wasn't there
> ---
>  test/ipa/rkisp1/rkisp1-utils.cpp | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp
> index e48f8d362..b1863894c 100644
> --- a/test/ipa/rkisp1/rkisp1-utils.cpp
> +++ b/test/ipa/rkisp1/rkisp1-utils.cpp
> @@ -21,7 +21,23 @@ using namespace ipa::rkisp1;
>  class RkISP1UtilsTest : public Test
>  {
>  protected:
> -       template<unsigned int IntPrec, unsigned FracPrec, typename T>
> +       /* R for real, I for integer */
> +       template<unsigned int IntPrec, unsigned int FracPrec, typename I, typename R>
> +       int testFixedToFloat(I input, R expected)
> +       {
> +               R out = utils::fixedToFloatingPoint<IntPrec, FracPrec, R>(input);
> +               R prec = 1.0 / (1 << FracPrec);
> +               if (std::abs(out - expected) > prec) {
> +                       cerr << "Reverse conversion expected " << input
> +                            << " to convert to " << expected
> +                            << ", got " << out << std::endl;
> +                       return TestFail;
> +               }
> +
> +               return TestPass;
> +       }
> +
> +       template<unsigned int IntPrec, unsigned int FracPrec, typename T>
>         int testSingleFixedPoint(double input, T expected)
>         {
>                 T ret = utils::floatingToFixedPoint<IntPrec, FracPrec, T>(input);
> @@ -54,7 +70,6 @@ protected:
>                  */
>                 std::map<double, uint16_t> testCases = {
>                         { 7.992, 0x3ff },
> -                       { 7.992, 0xbff },
>                         {   0.2, 0x01a },
>                         {  -0.2, 0x7e6 },
>                         {  -0.8, 0x79a },
> @@ -72,6 +87,11 @@ protected:
>                                 return ret;
>                 }
>  
> +               /* Special case with a superfluous one in the unused bits */
> +               ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);
> +               if (ret != TestPass)
> +                       return ret;
> +
>                 return TestPass;
>         }
>  
> -- 
> 2.39.2
>

Quoting Kieran Bingham (2024-06-10 11:00:17)
> Quoting Paul Elder (2024-06-07 11:03:23)
> > There was an issue where using map to store the test cases meant that
> > the test for ignoring unused bits was skipped because of clashing keys.
> > Fix this by moving the offending test out of the loop.
> > 
> > While at it, also change the arbitrary floating comparison precision to
> > be more precise.
> > 
> > Also fix a missing documentation brief.
> > 
> > Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to
> > fixed-point
> 
> I think it's better to keep trailier lines on a single line even if it
> extends beyond 72/80 chars.
> 
> I think the title is normally in brackets and quotes too?
> 
> Fixes: 9d152e9c6 ("ipa: rkisp1: Add a helper to convert floating-point to fixed-point")
> 
> But just trivial nits. That doesn't seem to be enforced by checkstyle
> presently.

Hrm ... I take that back - checkstyle certainly does have a rule for
this:



commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \(".*"\)')

    known_trailers = {
    	...
        'Fixes': commit_regex,
	...
    }

So - it should be enforced to be:

Fixes: 9d152e9c66c1 ("ipa: rkisp1: Add a helper to convert floating-point to fixed-point")

--
Kieran


> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - remove the added complexity that would've made it clearner to add a
> >   larger variety of tests because the variety wasn't there
> > ---
> >  test/ipa/rkisp1/rkisp1-utils.cpp | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp
> > index e48f8d362..b1863894c 100644
> > --- a/test/ipa/rkisp1/rkisp1-utils.cpp
> > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp
> > @@ -21,7 +21,23 @@ using namespace ipa::rkisp1;
> >  class RkISP1UtilsTest : public Test
> >  {
> >  protected:
> > -       template<unsigned int IntPrec, unsigned FracPrec, typename T>
> > +       /* R for real, I for integer */
> > +       template<unsigned int IntPrec, unsigned int FracPrec, typename I, typename R>
> > +       int testFixedToFloat(I input, R expected)
> > +       {
> > +               R out = utils::fixedToFloatingPoint<IntPrec, FracPrec, R>(input);
> > +               R prec = 1.0 / (1 << FracPrec);
> > +               if (std::abs(out - expected) > prec) {
> > +                       cerr << "Reverse conversion expected " << input
> > +                            << " to convert to " << expected
> > +                            << ", got " << out << std::endl;
> > +                       return TestFail;
> > +               }
> > +
> > +               return TestPass;
> > +       }
> > +
> > +       template<unsigned int IntPrec, unsigned int FracPrec, typename T>
> >         int testSingleFixedPoint(double input, T expected)
> >         {
> >                 T ret = utils::floatingToFixedPoint<IntPrec, FracPrec, T>(input);
> > @@ -54,7 +70,6 @@ protected:
> >                  */
> >                 std::map<double, uint16_t> testCases = {
> >                         { 7.992, 0x3ff },
> > -                       { 7.992, 0xbff },
> >                         {   0.2, 0x01a },
> >                         {  -0.2, 0x7e6 },
> >                         {  -0.8, 0x79a },
> > @@ -72,6 +87,11 @@ protected:
> >                                 return ret;
> >                 }
> >  
> > +               /* Special case with a superfluous one in the unused bits */
> > +               ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);
> > +               if (ret != TestPass)
> > +                       return ret;
> > +
> >                 return TestPass;
> >         }
> >  
> > -- 
> > 2.39.2
> >