On 2021-08-29 06:41, Liam Beguin wrote:

On Thu, Aug 26, 2021 at 11:53:14AM +0200, Peter Rosin wrote:

quoted

On 2021-08-24 22:28, Liam Beguin wrote:

quoted

On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:

quoted

[I started to write an answer to your plans in the v7 thread, but didn't
have time to finish before v8 appeared...]

On 2021-08-20 21:17, Liam Beguin wrote:

quoted

From: Liam Beguin <redacted>

The approximation caused by integer divisions can be costly on smaller
scale values since the decimal part is significant compared to the
integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
cases to maintain accuracy.

Hi Peter,

Thanks for taking time to look at this in detail again. I really
appreciate all the feedback you've provided.

quoted

The conversion to int-plus-nano may also carry a cost of accuracy.

90/1373754273 scaled by 261/509 is 3.359e-8, the old code returns 3.348e-8,
but the new one gets you 3.3e-8 (0.000000033, it simply cannot provide more
digits). So, in this case you lose precision with the new code.

Similar problem with 100 / 2^30 scaled by 3782/7000. It is 5.032e-8, the old
code returns 5.029e-8, but the new one gets you the inferior 5.0e-8.

I see what you mean here.
I added test cases with these values to see exactly what we get.

Excellent!

quoted

Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000033594
# iio_rescale_test_scale: not ok 42 - v8 - 90/1373754273 scaled by 261/509
Expected rel_ppm < 0, but
    rel_ppm == 1000000

     real=0.000000000
 expected=0.000000050318
# iio_rescale_test_scale: not ok 43 - v8 - 100/1073741824 scaled by 3782/7000


The main issue is that the first two examples return 0 which night be worst
that loosing a little precision.

They shouldn't return zero?

Here's the new code quoted from the test robot (and assuming
a 64-bit machine, thus ignoring the 32-bit problem on line 56).

    36		case IIO_VAL_FRACTIONAL:
    37		case IIO_VAL_FRACTIONAL_LOG2:
    38			tmp = (s64)*val * 1000000000LL;
    39			tmp = div_s64(tmp, rescale->denominator);
    40			tmp *= rescale->numerator;
    41	
    42			tmp = div_s64_rem(tmp, 1000000000LL, &rem);
    43			*val = tmp;
    44	
    45			/*
    46			 * For small values, the approximation can be costly,
    47			 * change scale type to maintain accuracy.
    48			 *
    49			 * 100 vs. 10000000 NANO caps the error to about 100 ppm.
    50			 */
    51			if (scale_type == IIO_VAL_FRACTIONAL)
    52				tmp = *val2;
    53			else
    54				tmp = 1 << *val2;
    55	
  > 56			 if (abs(rem) > 10000000 && abs(*val / tmp) < 100) {
    57				 *val = div_s64_rem(*val, tmp, &rem2);
    58	
    59				 *val2 = div_s64(rem, tmp);
    60				 if (rem2)
    61					 *val2 += div_s64(rem2 * 1000000000LL, tmp);
    62	
    63				 return IIO_VAL_INT_PLUS_NANO;
    64			 }
    65	
    66			return scale_type;

When I go through the above manually, I get:

line 
38: tmp = 90000000000    ; 90 * 1000000000
39: tmp = 176817288      ; 90000000000 / 509
40: tmp = 46149312168    ; 176817288 * 261
42: rem = 149312168      ; 46149312168 % 1000000000
    tmp = 46             ; 46149312168 / 1000000000
43: *val = 46
51: if (<fractional>) [yes]
52: tmp = 1373754273
56: if (149312168 > 10000000 && 46/1373754273 < 100) [yes && yes]
57: rem2 = 46            ; 46 % 1373754273
    *val = 0             ; 46 / 1373754273
59: *val2 = 0            ; 149312168 / 1373754273
60: if 46 [yes]
61: *val2 = 33           ; 0 + 46 * 1000000000 / 1373754273
63: return <int-plus-nano> [0.000000033]

and

line 
38: tmp = 100000000000   ; 100 * 1000000000
39: tmp = 14285714       ; 100000000000 / 7000
40: tmp = 54028570348    ; 176817288 * 3782
42: rem = 28570348       ; 54028570348 % 1000000000
    tmp = 54             ; 54028570348 / 1000000000
43: *val = 54
51: if (<fractional>) [no]
54: tmp = 1073741824     ; 1 << 30
56: if (28570348 > 10000000 && 54/1073741824 < 100) [yes && yes]
57: rem2 = 54            ; 54 % 1073741824
    *val = 0             ; 54 / 1073741824
59: *val2 = 0            ; 28570348 / 1073741824
60: if 46 [yes]
61: *val2 = 50           ; 0 + 54 * 1000000000 / 1073741824
63: return <int-plus-nano> [0.000000050]

Why do you get zero, what am I missing?

So... It turns out, I swapped schan and rescaler values which gives us:

Ahh, good. The explanation is simple!

numerator = 90
denominator = 1373754273
schan_val = 261
schan_val2 = 509

line
38: tmp = 261000000000   ; 261 * 1000000000
39: tmp = 189            ; 261000000000 / 1373754273
40: tmp = 17010          ; 189 * 90
42: rem = 17010          ; 17010 % 1000000000
    tmp = 0              ; 17010 / 1000000000
43: *val = 0
51: if (<fractional>) [yes]
52: tmp = 509
56: if (17010 > 10000000 && 0/509 < 100) [no && yes]
66: *val = 0
    *val2 = 509
    return <fractional> [0.000000000]

Swapping back the values, I get the same results as you!

Also, replacing line 56 from the snippet above with

	- if (abs(rem) > 10000000 && abs(div64_s64(*val, tmp)) < 100) {
	+ if (abs(rem)) {

Fixes these precision errors. It also prevents us from returning
different scales if we swap the two divisions (schan and rescaler
parameters).

No, it doesn't fix the precision problems. Not really, it only reduces
them. See below.

*snip*

quoted

quoted

Considering these null values and the possible issue of not always having the
same scale type, would it be better to always return an IIO_VAL_INT_PLUS_NANO
scale?

No, that absolutely kills the precision for small values that are much
better off as-is. The closer you get to zero, the more the conversion
to int-plus-nano hurts, relatively speaking.

I'm not sure I understand what you mean. The point of switching to
IIO_VAL_INT_PLUS_NANO at the moment is to get more precision on small
values. Am I missing something?

Yes, apparently :-) We always sacrifice accuracy by going to
IIO_VAL_INT_PLUS_NANO. More is lost with smaller numbers, relatively.
That is an inherent property of fix-point style representations such
as IIO_VAL_INT_PLUS_NANO. Unless we get lucky and just happen to be
able to represent the desired number exactly of course, but that tends
to be both non-interesting and the exception.

Let's go back to the example from my response to the 8/14 patch, i.e.
5/32768 scaled by 3/10000. With the current code this yields

15 / 327680000 (0.0000000457763671875)

Note, the above is /exact/. With IIO_VAL_INT_PLUS_NANO we instead get
the truncated 0.000000045

The relative error introduced by the IIO_VAL_INT_PLUS_NANO conversion
is almost 1.7% in this case. Sure, rounding instead of truncating
would reduce that to 0.5%, but that's not really a significant
improvement if you compare to /no/ error. Besides, there are many
smaller numbers with even bigger relative conversion "noise".

And remember, this function is used to rescale the scale of the
raw values. We are going to multiply the scale and the raw values
at some point. If we have rounding errors in the scale, they will
multiply with the raw values. It wouldn't look too good if something
that should be able to reach 3V with a lot of accuracy (ca 26 bits)
instead caps out at 2.94912V (or hits 3.014656V) because of accuracy
issues with the scaling (1.7% too low or 0.5% too high).

It's impossible to do better than exact, which is what we have now for
IIO_VAL_FRACTIONAL and IIO_VAL_INT (for IIO_VAL_FRACTIONAL_LOG2, not
so much...). At least as long as there's no overflow.

Cheers,
Peter