Re: testprofile on i686

[Robert Helling via subsurface]

Alan,

> On 9. Dec 2021, at 17:39, Alan Brown via subsurface 
>  wrote:
> 
> I see the PR has now been merged. The problem has been with the
> calculation of EAD and END. I am assuming these stand for Equivalent Air
> Depth and Equivalent Narcotic Depth. I believe the Bühlmann ZH16
> algorithm does not use EADs when calculating decompression requirements
> and the EAD calculations are for providing info when viewing the dive
> profile. Is this correct?

Your assumptions are correct. The calculations we were looking at were only for 
the profile and the information box. They are relevant when you chose your 
breathing gases not so much for decompression calculations which is what the 
Bühlmann model is about (well, that’s not entirely true: The EAD tells you that 
instead of the nitrox gas you are actually breathing you could equivalently 
calculate the decompression for air but at a modified depth with is the EAD).

The planner does the inverse calculations if you ask it to select a „best mix“: 
Given a profile, that gas mix has the maximum O2 fraction that does not exceed 
the allowable partial pressure of O2 (typically 1.4-1.6 bar) and/or chooses the 
He content such that you will never breath a gas more narcotic than air at a 
prescribed END (typically 30m).

Best
Robert


signature.asc
Description: Message signed with OpenPGP
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

Robert,

Alan Brown via subsurface  writes:
> The fgetround test produces the following
>
> rounding using to-nearest rounding:
> rint (2.3) = 2.0
> rint (3.8) = 4.0
> rint (-2.3) = -2.0
> rint (-3.8) = -4.0

I see the PR has now been merged. The problem has been with the
calculation of EAD and END. I am assuming these stand for Equivalent Air
Depth and Equivalent Narcotic Depth. I believe the Bühlmann ZH16
algorithm does not use EADs when calculating decompression requirements
and the EAD calculations are for providing info when viewing the dive
profile. Is this correct?

Thanks

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

Robert Helling  writes:

>
>  depth * 1000.0 / 1000.0 which for a depth of 30m seemed to end up just a 
> tiny bit below 30.
>
>  That implies that we (once again) didn't do proper rounding.
>
>  I suspect just short-circuiting it for zero helium hides the problem
>  rather than fixing it.
>
>   Linus
>
> what does the test program on 
> https://www.cplusplus.com/reference/cfenv/fegetround/ produce on your 
> machine? I am a but afraid to explicitly set the rounding mode just to be 
> sure as that
> (according to my understanding of the docs I found) might cause the compiler 
> to think we are doing strange stuff.

The fgetround test produces the following

rounding using to-nearest rounding:
rint (2.3) = 2.0
rint (3.8) = 4.0
rint (-2.3) = -2.0
rint (-3.8) = -4.0

The latest patch fixes the rounding errors on i686. And now it passes
all of the tests for all of the architectures that the build process
tests on.

https://github.com/void-linux/void-packages/pull/34187

I am happy to continue testing things here as well though.

One of the pull request moderators has patched libdivecomputer because
when building for musl some dive computers were showing warnings

--snip--
oceanic_common.h:31: warning: "PAGESIZE" redefined
   31 | #define PAGESIZE 0x10
--snip--

regards

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

Robert Helling  writes:

>
>  I suspect just short-circuiting it for zero helium hides the problem
>  rather than fixing it.
>
>   Linus
>
> what does the test program on 
> https://www.cplusplus.com/reference/cfenv/fegetround/ produce on your 
> machine? I am a but afraid to explicitly set
> the rounding mode just to be sure as that (according to my understanding of 
> the docs I found) might cause the compiler to think we are doing strange 
> stuff.

Robert, Linus,

Happy to do the tests, but might be a couple of days. I am travelling
all day today and it is unlikely I will be able to look at this evening.

Regards

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Robert Helling via subsurface]

Alan,

> On 5. Dec 2021, at 23:07, Linus Torvalds  
> wrote:
> 
> On Sun, Dec 5, 2021 at 2:03 PM Robert Helling via subsurface
>  wrote:
>> 
>> depth * 1000.0 / 1000.0 which for a depth of 30m seemed to end up just a 
>> tiny bit below 30.
> 
> That implies that we (once again) didn't do proper rounding.
> 
> I suspect just short-circuiting it for zero helium hides the problem
> rather than fixing it.
> 
>  Linus

what does the test program on 
https://www.cplusplus.com/reference/cfenv/fegetround/ 
 produce on your 
machine? I am a but afraid to explicitly set the rounding mode just to be sure 
as that (according to my understanding of the docs I found) might cause the 
compiler to think we are doing strange stuff.

Best
Robert


signature.asc
Description: Message signed with OpenPGP
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Linus Torvalds via subsurface]

On Sun, Dec 5, 2021 at 2:03 PM Robert Helling via subsurface
 wrote:
>
>  depth * 1000.0 / 1000.0 which for a depth of 30m seemed to end up just a 
> tiny bit below 30.

That implies that we (once again) didn't do proper rounding.

I suspect just short-circuiting it for zero helium hides the problem
rather than fixing it.

  Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Robert Helling via subsurface]

Alan,

> On 5. Dec 2021, at 21:42, Alan Brown via subsurface 
>  wrote:
> 
> It looks like it failed again, but we are down to just a couple of
> differences. It looks like they are with END (certainly on
> exportprofile) rather than EAD.
> 
> https://gist.github.com/adbrown101/4f891b99e501b0ba20889c656f76a43d 
> 
> 
> https://gist.github.com/adbrown101/ac48b9c3ca56f9e3d35bcfb540a8fa08 
> 
> 
> Definitely getting there with this particular issue

I added another commit. The problematic cases seemed to come from calculations 
of END for dives without helium where END should simply be the current depth 
but there we did depth * 1000.0 / 1000.0 which for a depth of 30m seemed to end 
up just a tiny bit below 30. So this commit short circuits this calculation in 
case of zero helium.

Could you please try again to see if this eliminates the remaining cases?

Best
Robert



signature.asc
Description: Message signed with OpenPGP
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

"Robert.Helling"  writes:

>
> Yes, that is related. Rather than simply adding/subtracting 1 to convert 
> between depth in mm and ambient pressure, these functions properly take into 
> account the surface pressure (not
> exactly 1 bar)  and the density of water (not exactly 1000 kg/m^3). But these 
> functions use Subsurface internal unit types which are integer Millimeters 
> and integer millibars. As Berthold pointed
> out, the letter have a quantisation that amounts to 1cm of depth and these 
> seem to be the differences we are seeing.
>
> I pushed another version of the the pull request which does not internally 
> quantise to integer bars which might behave better.
>
> Could you please try this again with the current version. That would be great.
>
Robert,

It looks like it failed again, but we are down to just a couple of
differences. It looks like they are with END (certainly on
exportprofile) rather than EAD.

https://gist.github.com/adbrown101/4f891b99e501b0ba20889c656f76a43d

https://gist.github.com/adbrown101/ac48b9c3ca56f9e3d35bcfb540a8fa08

Definitely getting there with this particular issue

Regards

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Robert.Helling via subsurface]

Hi everyone,

> On 5. Dec 2021, at 00:16, Alan Brown via subsurface 
>  wrote:
> 
> Is this related to the warnings I am seeing in the build log, which I am
> seeing following the patch?
> 
> --snip--
> /home/alan/src/subsurface/core/profile.c: In function 
> ‘calculate_gas_information_new’:
> /home/alan/src/subsurface/core/profile.c:1259:79: warning: conversion from 
> ‘double’ to ‘int’ may change value [-Wfloat-conversion]
> 1259 |   entry->end = mbar_to_depth(depth_to_mbar(entry->depth, dive) * (1000 
> - fhe) / 1000.0, dive);
>  |  
> ~^~~~
> /home/alan/src/subsurface/core/profile.c:1260:70: warning: conversion from 
> ‘double’ to ‘int’ may change value [-Wfloat-conversion]
> 1260 |   entry->ead = mbar_to_depth(depth_to_mbar(entry->depth, dive) * fn2 / 
> (double)N2_IN_AIR, dive);
> /home/alan/src/subsurface/core/profile.c:1265:61: warning: conversion from 
> ‘double’ to ‘int’ may change value [-Wfloat-conversion]
> 1261 |   entry->eadd = mbar_to_depth(depth_to_mbar(entry->depth, dive) *
>  |   ~~~
> 1262 |   (entry->pressures.o2 / amb_pressure * O2_DENSITY +
>  |   ~~
> 1263 |entry->pressures.n2 / amb_pressure * N2_DENSITY +
>  |~
> 1264 |entry->pressures.he / amb_pressure * HE_DENSITY) /
>  |~~
> 1265 |   (O2_IN_AIR * O2_DENSITY + N2_IN_AIR * N2_DENSITY) * 1000, 
> dive);
>  |   ~~^~
> --snip--

Yes, that is related. Rather than simply adding/subtracting 1 to convert 
between depth in mm and ambient pressure, these functions properly take into 
account the surface pressure (not exactly 1 bar)  and the density of water (not 
exactly 1000 kg/m^3). But these functions use Subsurface internal unit types 
which are integer Millimeters and integer millibars. As Berthold pointed out, 
the letter have a quantisation that amounts to 1cm of depth and these seem to 
be the differences we are seeing. 

I pushed another version of the the pull request which does not internally 
quantise to integer bars which might behave better. 

> 
> For my own sanity I used qemu to build a debian 32 bit machine and did a
> subsurface build from the master branch using the patch and TestProfile
> failed with the same diffs.

Could you please try this again with the current version. That would be great.

Best
Robert


___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

"Robert.Helling"  writes:

> Hi,
>
>  On 4. Dec 2021, at 13:09, Berthold Stoeger via subsurface 
>  wrote:
>
>  Yes, in particular Robert's new code reads as
>
>  entry->ead = mbar_to_depth(depth_to_mbar(entry->depth, dive) ...);
>
>  depth_to_mbar() returns an int, and 1 bar is 10 m, thus 1 mbar is 10 mm which
>  is precisely the observed difference, no?
>
> I could add easily a double valued version of that function which should 
> solve this.
>
Robert, Berthold

Is this related to the warnings I am seeing in the build log, which I am
seeing following the patch?

--snip--
/home/alan/src/subsurface/core/profile.c: In function 
‘calculate_gas_information_new’:
/home/alan/src/subsurface/core/profile.c:1259:79: warning: conversion from 
‘double’ to ‘int’ may change value [-Wfloat-conversion]
 1259 |   entry->end = mbar_to_depth(depth_to_mbar(entry->depth, dive) * (1000 
- fhe) / 1000.0, dive);
  |  
~^~~~
/home/alan/src/subsurface/core/profile.c:1260:70: warning: conversion from 
‘double’ to ‘int’ may change value [-Wfloat-conversion]
 1260 |   entry->ead = mbar_to_depth(depth_to_mbar(entry->depth, dive) * fn2 / 
(double)N2_IN_AIR, dive);
/home/alan/src/subsurface/core/profile.c:1265:61: warning: conversion from 
‘double’ to ‘int’ may change value [-Wfloat-conversion]
 1261 |   entry->eadd = mbar_to_depth(depth_to_mbar(entry->depth, dive) *
  |   ~~~
 1262 |   (entry->pressures.o2 / amb_pressure * O2_DENSITY +
  |   ~~
 1263 |entry->pressures.n2 / amb_pressure * N2_DENSITY +
  |~
 1264 |entry->pressures.he / amb_pressure * HE_DENSITY) /
  |~~
 1265 |   (O2_IN_AIR * O2_DENSITY + N2_IN_AIR * N2_DENSITY) * 1000, 
dive);
  |   ~~^~
--snip--

For my own sanity I used qemu to build a debian 32 bit machine and did a
subsurface build from the master branch using the patch and TestProfile
failed with the same diffs.

Regards

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Robert.Helling via subsurface]

Hi,

> On 4. Dec 2021, at 13:09, Berthold Stoeger via subsurface 
>  wrote:
> 
> Yes, in particular Robert's new code reads as
> 
> entry->ead = mbar_to_depth(depth_to_mbar(entry->depth, dive) ...);
> 
> depth_to_mbar() returns an int, and 1 bar is 10 m, thus 1 mbar is 10 mm which 
> is precisely the observed difference, no?

I could add easily a double valued version of that function which should solve 
this.

But there is another concern that I have and that I would like to hear people’s 
opinion on: In the previous version of the code, this conversion was simply 
done by adding/subtracting  1 as everybody is doing this when doing the 
conversion in their head (pressure in bar is depth in meters divided by 10 plus 
1). But our conversion functions are more sophisticated: They take into account 
the actual surface pressure (when known) and the salinity/density of water (if 
know). The latter does not matter here since it cancels out when we convert 
back and forth but the surface pressure not being exactly 1 bar matters 
typically at the percent level.

My question is: Do you think this is the right thing to do? Of course, our 
functions fo a more accurate job at the conversion between depth and ambient 
pressure. But do we really want this? There is a chance that this leads to 
complaints that we are computing wrong values as our value does not agree with 
what our users might compute with the simpler conversion (as we constantly get 
complaints that we are getting gas consumption wrong because people user the 
ideal gas law rather than our more correct real gas version). But even worse, 
despite the „D“ in the name of the numbers standing for „depth“, is it really 
depth that we mean or is it actually ambient pressure (as for the physiological 
effects it is most likely (partial) pressure that matters. Given that the dive 
computer also measures pressure not depth is for example the MOD really the 
„maximum operating depth“ or should it rather be the „maximum operating ambient 
pressure“?

Of course „doesn’t matter“ is a valid answer since the differences are at the 
order of a percent and diving is an inexact science and none of the things that 
happen are known/understood/measured at the 1% level.

A totally different question is if it really makes sense to output these values 
involving floating point numbers (possibly rounded at some point with the 
resulting discontinuities) as a text file or should the test better read the 
resulting csv files and compare numbers with some tolerance? I remember a 
number of years ago, we went though the code and removed all tests of floating 
point numbers for equality and replaced those to a comparison with finite 
margin of error.

Best
Robert
 ___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Berthold Stoeger via subsurface]

On Freitag, 3. Dezember 2021 23:12:39 CET Linus Torvalds via subsurface wrote:
> On Fri, Dec 3, 2021 at 1:48 PM Alan Brown via subsurface
> 
>  wrote:
> > Thanks for looking into this. The test still fails but the number of
> > lines in the diffs are fewer.
> 
> Still seems to be that EAD line if I count the columns right.
> 
> We do have a few places where we end up converting to an integer
> value, only to then convert back to a floating point.

Yes, in particular Robert's new code reads as

entry->ead = mbar_to_depth(depth_to_mbar(entry->depth, dive) ...);

depth_to_mbar() returns an int, and 1 bar is 10 m, thus 1 mbar is 10 mm which 
is precisely the observed difference, no?

The whole thing is a bit of a mess.

Berthold



___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Linus Torvalds via subsurface]

On Fri, Dec 3, 2021 at 1:48 PM Alan Brown via subsurface
 wrote:
> Thanks for looking into this. The test still fails but the number of
> lines in the diffs are fewer.

Still seems to be that EAD line if I count the columns right.

We do have a few places where we end up converting to an integer
value, only to then convert back to a floating point.

Fore example, depth_to_bar() returns a 'double', but we do things the
wrong way around - doing the calculations in floating point in
calculate_depth_to_mbar(), but returning things as a integer 'mbar',
and then depth_to_bar() does

return depth_to_mbar(depth, dive) / 1000.0;

and I think the reason for that is historical - depth_to_bar() came
later, to do deco calculations, and it thus used the (existing)
"integer mbar" function that we use.

End result: we have some unnecessary quantization of values in the
middle of calculations.

That said, it still smells like some missing rounding, because I don't
see why you'd get different values on i387 otherwise.

And I don't see where it would be.

   Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

Robert Helling via subsurface  writes:

> this is an excellent observation. I completely missed the fact that for no 
> good reason, the fraction fn2 and fhe are rounded to integers. As 
> intermediate values, they should be kept double. While I
> was at it, I made MOD, END, EAD, EAAD integers since those are all depths and 
> thus they should be mm in ints.
>
> My hope would now be that rounded to mm, the difference in floating point 
> precision should not really show up (at least in most of the cases).
>
> Alan, would you please check if my PR passes the tests on your architectures?
>
> https://github.com/subsurface/subsurface/pull/3357
>
Robert,

Thanks for looking into this. The test still fails but the number of
lines in the diffs are fewer.

https://gist.github.com/adbrown101/db61dd3c61861846f3c5f8a573d974ef

https://gist.github.com/adbrown101/b5ce9440827e3d3ff975beb39f93e32d

It looks like the numbers are out  by 10.

Regards

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Robert Helling via subsurface]

Hi everyone,

> On 3. Dec 2021, at 12:58, Berthold Stoeger via subsurface 
>  wrote:
> 
> Indeed, by rounding one down, I get the "alternative" value. And of course,
> ceil() and floor() would exhibit the same instability depending on whether you
> are eps above or below the integer. So I reckon you should use round(...) or
> lrint(…)
> .


this is an excellent observation. I completely missed the fact that for no good 
reason, the fraction fn2 and fhe are rounded to integers. As intermediate 
values, they should be kept double. While I was at it, I made MOD, END, EAD, 
EAAD integers since those are all depths and thus they should be mm in ints.

My hope would now be that rounded to mm, the difference in floating point 
precision should not really show up (at least in most of the cases).

Alan, would you please check if my PR passes the tests on your architectures?

https://github.com/subsurface/subsurface/pull/3357 


Best
Robert



signature.asc
Description: Message signed with OpenPGP
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Berthold Stoeger via subsurface]

On Freitag, 3. Dezember 2021 12:49:48 CET Berthold Stoeger via subsurface 
wrote:
> Hi Robert,
> 
> On Donnerstag, 2. Dezember 2021 08:38:25 CET Robert Helling via subsurface
> 
> wrote:
> > fn2 = (int)(1000.0 * entry->pressures.n2 /
> 
> amb_pressure);
> 
> This looks ill-defined. You probably want lrint(...) or (int)(round(...)) or
> (int)(ceil(...)) or (int)(floor(...)) for reproducible behavior.

Indeed, by rounding one down, I get the "alternative" value. And of course, 
ceil() and floor() would exhibit the same instability depending on whether you 
are eps above or below the integer. So I reckon you should use round(...) or 
lrint(...).

Berthold


___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Berthold Stoeger via subsurface]

Hi Robert,

On Donnerstag, 2. Dezember 2021 08:38:25 CET Robert Helling via subsurface 
wrote:

>   fn2 = (int)(1000.0 * entry->pressures.n2 / 
amb_pressure);

This looks ill-defined. You probably want lrint(...) or (int)(round(...)) or  
(int)(ceil(...)) or (int)(floor(...)) for reproducible behavior.

The whole code looks fishy to me (why round to int in the first place?).

Berthold


___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Thiago Macieira via subsurface]

On Wednesday, 1 December 2021 14:21:28 PST Linus Torvalds via subsurface 
wrote:
> I'm not seeing what's going on, but it smells like some instability in
> the calculations rather than some extra non-ieee precision of the
> i387.

An easy way to verify if it is the 387 extra precision is to remove it: add 
-msse2 -mfpmath=sse to your C/C++ build flags throughout Subsurface and unit 
test builds and see of the problem disappears.

BTW, Qt by default builds itself with those two flags on i386. Since no one 
has complained that Subsurface doesn't run on their 20+-year-old computer, we 
could turn it on for Subsurface too.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering



___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Robert Helling via subsurface]

Hi everybody,

> On 1. Dec 2021, at 23:21, Linus Torvalds  
> wrote:
> 
> It's certainly not a known issue, the FP precision thing was just a guess.
> 
> And honestly, the differences seem to be too big to be FP precision
> issues, particularly the i386 kind where the most common issue is that
> the calculations are done with a 64-bit mantissa rather than a 53 (or
> 24, for 'float') bit mantissa.
> 
> If I'm seeing things right (not a huge fan of CSV files with lots of
> columns), the EAD difference are things like 6407.426376 ->
> 6386.683739. Not exactly least-significant-digit kind of thing.
> 
> I'm not seeing what's going on, but it smells like some instability in
> the calculations rather than some extra non-ieee precision of the
> i387.
> 
> But it's also not any kind of *cumulative* error - looking at that
> diff, the very next line both files have an ead column with the same
> value (6508.706786).
> 
> Very strange. Why would that odd difference only show up on 32-bit i386?
> 
> Adding Robert to the cc, in case he sees what is up. I thionk all the
> EAD calculations are his code. Robert?

I have no idea where this could come from. the EAD is calculated in profile.c 
straight forwardly  as

entry->ead = (entry->depth + 1) * fn2 / (double)N2_IN_AIR - 
1;

and fn2 a few lines above as

fn2 = (int)(1000.0 * entry->pressures.n2 / amb_pressure);

In the diff, both versions agree that entry->depth is (I am looking at line 7 
of the CSV) 1830, entry->pressures.n2 is 0.947618, amp_pressure is 1.198000 and 
the macro N2_IN_AIR expands to 781. Plugging all this into my HP 45 SD, I get 
1981.472471 which is the value in the reference file.

Before subtracting 1, the difference between 1981.472471 and 1966.325224 
amounts to about 1.5% which should be far too much for floating point rounding 
error in such a calculation.

To investigate this further, I would place a breakpoint in line 1260 of 
profile.c and check how it arrives at the resulting value.

Best
Robert


signature.asc
Description: Message signed with OpenPGP
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Linus Torvalds via subsurface]

On Wed, Dec 1, 2021 at 12:53 PM Alan Brown via subsurface
 wrote:
>
> Scratch that, I hadn't chrooted into the build environment properly. The
> profiles in x86_64 built by TestProfile are identical to the reference
> files. Repeating the tests in the i686 environment does produce a lot of
> differences. The diffs are at.
>
> https://gist.github.com/adbrown101/82d218a7107520812768e4fed665aad4

Hmm. If I read that right, it's the "EAD" column that differs.

Maybe others do too.

> > That would explain why it fails on i686 but nowhere else: x87 floating 
> > point,
> > which keeps extra precision between operations that the other ones don't.
>
> Does this mean it is a known issue? I had planned to build a 32 bit
> version of another distribution to see if it is repeatable there. Would
> this be worthwhile?

It's certainly not a known issue, the FP precision thing was just a guess.

And honestly, the differences seem to be too big to be FP precision
issues, particularly the i386 kind where the most common issue is that
the calculations are done with a 64-bit mantissa rather than a 53 (or
24, for 'float') bit mantissa.

If I'm seeing things right (not a huge fan of CSV files with lots of
columns), the EAD difference are things like 6407.426376 ->
6386.683739. Not exactly least-significant-digit kind of thing.

I'm not seeing what's going on, but it smells like some instability in
the calculations rather than some extra non-ieee precision of the
i387.

But it's also not any kind of *cumulative* error - looking at that
diff, the very next line both files have an ead column with the same
value (6508.706786).

Very strange. Why would that odd difference only show up on 32-bit i386?

Adding Robert to the cc, in case he sees what is up. I thionk all the
EAD calculations are his code. Robert?

 Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

Alan Brown via subsurface  writes:

> When I look at the generated files in the build directory they are empty
> ls -al exportprofile*
> -rw-r--r-- 1 alan alan 0 Nov 29 16:23 exportprofile.csv
> -rw-r--r-- 1 alan alan 0 Nov 29 16:23 exportprofileVPMB.csv

Scratch that, I hadn't chrooted into the build environment properly. The
profiles in x86_64 built by TestProfile are identical to the reference
files. Repeating the tests in the i686 environment does produce a lot of
differences. The diffs are at.

https://gist.github.com/adbrown101/82d218a7107520812768e4fed665aad4

https://gist.github.com/adbrown101/0c9755453220ad337359f3652809323f

Thiago Macieira via subsurface  writes:
>
> That would explain why it fails on i686 but nowhere else: x87 floating point,
> which keeps extra precision between operations that the other ones don't.

Does this mean it is a known issue? I had planned to build a 32 bit
version of another distribution to see if it is repeatable there. Would
this be worthwhile?

regards

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Alan Brown via subsurface]

Linus Torvalds  writes:
> Rerun just that test:
>
> cd build/tests
> ./TestProfile
>
> and you should now have the two output files in that directory:
> exportprofile.csv and exportprofileVPMB.csv.
>
Thanks for you reply,

Ran this test on my x86_64 machine and got the result

* Start testing of TestProfile *
Config: Using QtTest library 5.15.2, Qt 5.15.2
(x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.2.1
20201203), void unknown
PASS   : TestProfile::initTestCase()
FAIL!  : TestProfile::testProfileExport() Compared values are not the same
   Actual   (org.open(QFile::ReadOnly)): 0
   Expected (true) : 1
   Loc: [/builddir/Subsurface-5.0.5/tests/testprofile.cpp(40)]
FAIL!  : TestProfile::testProfileExportVPMB() Compared values are not
the same
   Actual   (org.open(QFile::ReadOnly)): 0
   Expected (true) : 1
   Loc: [/builddir/Subsurface-5.0.5/tests/testprofile.cpp(56)]
PASS   : TestProfile::cleanupTestCase()
Totals: 2 passed, 2 failed, 0 skipped, 0 blacklisted, 2ms
* Finished testing of TestProfile *

When I look at the generated files in the build directory they are empty
ls -al exportprofile*
-rw-r--r-- 1 alan alan 0 Nov 29 16:23 exportprofile.csv
-rw-r--r-- 1 alan alan 0 Nov 29 16:23 exportprofileVPMB.csv

So these will not compare with the reference files. This architecture
 passed using the automated build and test system on void. So I think I
 will do a little more investigation to make sure I can rule out the
 void build system.

I do have a working version of Subsurface on my computer built this way
and I have found it the best way for logging my dives from my Suunto.

Thanks

Alan
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Thiago Macieira via subsurface]

On Sunday, 28 November 2021 08:50:04 PST Linus Torvalds via subsurface wrote:
> > I am trying to package version 5.0.5 for void linux. The build seems to
> > pass all of the tests for x86_64, aarch64, armv7l, armv6l but fails on
> > i686 on the testprofile. Does anyone know why this might be happening
> > specifically for this architecture?
> 
> At a guess, it's some floating point rounding issue or similar, and a
> failure of the test itself rather than the binary.

That would explain why it fails on i686 but nowhere else: x87 floating point, 
which keeps extra precision between operations that the other ones don't.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering



___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: testprofile on i686

[Linus Torvalds via subsurface]

On Sun, Nov 28, 2021 at 8:29 AM Alan Brown via subsurface
 wrote:
>
> I am trying to package version 5.0.5 for void linux. The build seems to
> pass all of the tests for x86_64, aarch64, armv7l, armv6l but fails on
> i686 on the testprofile. Does anyone know why this might be happening
> specifically for this architecture?

At a guess, it's some floating point rounding issue or similar, and a
failure of the test itself rather than the binary.

subsurface actually avoids using floating point for most things (most
data being kept as integers in well-defined formats - like "depth in
mm" etc).

But things like the deco algorithms do use floating point, so I could
see that causing issues for things like calculated profiles etc.

The other failure mode we've seen is just silly locale differences -
we've had bugs like printing out floating point numbers using ',' vs
'.' etc.

Anyway, regardless of what the cause of the failure is, what you can
do is something like this:

Rerun just that test:

cd build/tests
./TestProfile

and you should now have the two output files in that directory:
exportprofile.csv and exportprofileVPMB.csv.

The tests then compare that to the expected end results that are in
the 'dives' directory in the source tree, so this *should* give an
empty diff

   diff -u ../../dives/exportprofilereferenceVPMB.csv exportprofileVPMB.csv

but clearly that part fails.

And what would be interesting to see is the difference between the
reference file and the generated file.

Don't send the whole diff to the mailing list in case it's "every line
differs in one character" kind of thing. But if you can pinpoint more
precisely what the difference is, we can figure it out, I'm sure.

I don't have a 32-bit environment any more (and it might not be about
x86-64 vs i386: as mentioned it could be something like an unusual
locale thing too, and might be specific to void for some odd reason).

  Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface