LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peter Rosin <peda@axentia.se>
To: Liam Beguin <liambeguin@gmail.com>
Cc: jic23@kernel.org, lars@metafoo.de, linux-kernel@vger.kernel.org,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	robh+dt@kernel.org
Subject: Re: [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small
Date: Mon, 30 Aug 2021 15:03:52 +0200	[thread overview]
Message-ID: <d714a5ca-07d6-1e6b-53d6-3353fe24e485@axentia.se> (raw)
In-Reply-To: <YSsP8aWU2uFBOt6M@shaak>

On 2021-08-29 06:41, Liam Beguin wrote:
> On Thu, Aug 26, 2021 at 11:53:14AM +0200, Peter Rosin wrote:
>> On 2021-08-24 22:28, Liam Beguin wrote:
>>> On Mon Aug 23, 2021 at 00:18:55 +0200, Peter Rosin wrote:
>>>> [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:
>>>>> From: Liam Beguin <lvb@xiphos.com>
>>>>>
>>>>> 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.
>>>
>>>> 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!
>>
>>>
>>> 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*

>>> 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

  parent reply	other threads:[~2021-08-30 13:04 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 19:17 [PATCH v8 00/14] iio: afe: add temperature rescaling support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 01/14] iio: inkern: apply consumer scale on IIO_VAL_INT cases Liam Beguin
2021-08-20 19:17 ` [PATCH v8 02/14] iio: inkern: apply consumer scale when no channel scale is available Liam Beguin
2021-08-20 19:17 ` [PATCH v8 03/14] iio: inkern: make a best effort on offset calculation Liam Beguin
2021-08-20 19:17 ` [PATCH v8 04/14] iio: afe: rescale: expose scale processing function Liam Beguin
2021-08-20 19:17 ` [PATCH v8 05/14] iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support Liam Beguin
2021-08-26  8:11   ` Peter Rosin
2021-08-29  2:50     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 06/14] iio: afe: rescale: add offset support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 07/14] iio: afe: rescale: use s64 for temporary scale calculations Liam Beguin
2021-08-20 19:17 ` [PATCH v8 08/14] iio: afe: rescale: reduce risk of integer overflow Liam Beguin
2021-08-26  9:13   ` Peter Rosin
2021-08-29  4:01     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales Liam Beguin
2021-08-20 23:37   ` kernel test robot
2021-08-21  1:33     ` Liam Beguin
2021-08-23  6:53       ` Peter Rosin
2021-08-21  2:00   ` kernel test robot
2021-08-21  7:21   ` kernel test robot
2021-08-22 22:18   ` Peter Rosin
2021-08-24 20:28     ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small Liam Beguin
2021-08-26  9:53       ` Peter Rosin
2021-08-29  4:41         ` Liam Beguin
2021-08-30 11:27           ` Jonathan Cameron
2021-09-11 23:31             ` Liam Beguin
2021-08-30 13:03           ` Peter Rosin [this message]
2021-09-11 23:20             ` Liam Beguin
2021-08-30 11:22     ` [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small fractional scales Jonathan Cameron
2021-08-30 14:30       ` Peter Rosin
2021-08-30 17:03         ` Jonathan Cameron
2021-09-02  2:27         ` Liam Beguin
2021-09-02 21:52           ` Peter Rosin
2021-08-20 19:17 ` [PATCH v8 10/14] iio: test: add basic tests for the iio-rescale driver Liam Beguin
2021-08-20 19:17 ` [PATCH v8 11/14] iio: afe: rescale: add RTD temperature sensor support Liam Beguin
2021-08-20 19:17 ` [PATCH v8 12/14] iio: afe: rescale: add temperature transducers Liam Beguin
2021-08-26  8:56   ` Peter Rosin
2021-08-29  2:33     ` Liam Beguin
2021-08-20 19:17 ` [PATCH v8 13/14] dt-bindings: iio: afe: add bindings for temperature-sense-rtd Liam Beguin
2021-08-20 19:17 ` [PATCH v8 14/14] dt-bindings: iio: afe: add bindings for temperature transducers Liam Beguin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d714a5ca-07d6-1e6b-53d6-3353fe24e485@axentia.se \
    --to=peda@axentia.se \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=liambeguin@gmail.com \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v8 09/14] iio: afe: rescale: fix accuracy for small' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).