Skip to content

sellipse/scircle overlaps regression tests #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
df7cb opened this issue Sep 5, 2023 · 7 comments · Fixed by #62
Closed

sellipse/scircle overlaps regression tests #61

df7cb opened this issue Sep 5, 2023 · 7 comments · Fixed by #62

Comments

@df7cb
Copy link
Contributor

df7cb commented Sep 5, 2023

Hi,

on 32-bit i386 Debian Linux I see a regression failure:

**** regression.diffs ****
diff -U3 /<<PKGBUILDDIR>>/expected/overlaps.out /<<PKGBUILDDIR>>/results/overlaps.out
--- /<<PKGBUILDDIR>>/expected/overlaps.out	2023-08-31 05:45:01.000000000 +0000
+++ /<<PKGBUILDDIR>>/results/overlaps.out	2023-09-05 10:13:42.121934808 +0000
@@ -963,7 +963,7 @@
 select 'sbox && sellipse', 'f' as expected, sbox'((-10d , -10d), (10d , 10d))' && sellipse'<{ 80d , 10d }, (0d, 90d) , 90d>' as actual;
      ?column?     | expected | actual 
 ------------------+----------+--------
- sbox && sellipse | f        | f
+ sbox && sellipse | f        | t
 (1 row)
 
 -- the ellipse lies completely in the box and they have an intersection at the boundary points
### End 15 installcheck (FAILED with exit code 1) ###

Also, this bit from 56ff366 looks fishy:

diff --git a/expected/overlaps.out b/expected/overlaps.out
index 5a7625f..5b1f885 100644
--- a/expected/overlaps.out
+++ b/expected/overlaps.out
@@ -835,14 +835,14 @@ select 'sbox && scircle', 'f' as expected, sbox'((0d , 0d), (20d , 20d))' && sci
 select 'sbox && scircle', 'f' as expected, sbox'((0d , 0d), (0d , 0d))' && scircle'<(10d , 0d) , 10d>' as actual;
     ?column?     | expected | actual
 -----------------+----------+--------
- sbox && scircle | f        | f
+ sbox && scircle | f        | t
 (1 row)

 -- the box degenerated into the point lies in the interior of circle
 select 'sbox && scircle', 'f' as expected, sbox'((0d , 0d), (0d , 0d))' && scircle'<(0d , 0d) , 10d>' as actual;
     ?column?     | expected | actual
 -----------------+----------+--------
- sbox && scircle | f        | f
+ sbox && scircle | f        | t
 (1 row)

 -- the box and circle are degenerated into the point and coincide

Why was the "actual" value updated? It's not failing, but if there is an "expected" column, they should match.

@vitcpp
Copy link
Contributor

vitcpp commented Sep 5, 2023

@df7cb The overlaps.sql test purpose is to highlight the differences between DE9-IM semantics of overlaps operation and the behaviour of pgsphere's overlap operation. The expected column contains the value as described in DE9-IM as it should be if pgsphere follows DE9-IM. The actual column shows the current result of the operation.

The differences in 'actual' column should be investigated and fixed. I'm not sure why it happens now. Definitely, we have to fix such difference in behaviour on 64 and 32 bit platforms. Thank you very much for reporting.

The patch 56ff366 fixes the read of unitialized memory that was revealed by overlaps.sql test when compiling with different optimization gcc settings (O0, O2). It is pretty simple fix. I don't exclude that same problem may appear in other places.

We have some plans to add more platforms for testing, including 32 bit platforms as well.

@vitcpp
Copy link
Contributor

vitcpp commented Sep 5, 2023

@df7cb I've run pgsphere tests on FreeBSD 13.2-RELEASE GENERIC i386 (without healpix). All tests are passed ok. Did you use 1.3.0 version (latest)? Which platform did you use to run the tests?

@vitcpp
Copy link
Contributor

vitcpp commented Sep 5, 2023

I reproduced the test fail on 32 bit Debian Linux on my virtual box.

@vitcpp
Copy link
Contributor

vitcpp commented Sep 7, 2023

The problem originates in FPge function. It may produce different results when using MMX on 64 bit and i32 FPU. There is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=323 bug. It can be fixed by gmake CFLAGS=-ffloat-store when compiling pgsphere but it may affect the performance. It seems that comparison functions FPeq/ne/lt/le/gt/ge should be slightly changed in case of 32 bit platforms. I'm working on it.

@vitcpp vitcpp linked a pull request Sep 7, 2023 that will close this issue
@df7cb
Copy link
Contributor Author

df7cb commented Sep 7, 2023

Hmm, -ffloat-store doesn't help here (I would have tried -fexcess-precision=standard, but that doesn't help either). It's great that you found something!

@vitcpp
Copy link
Contributor

vitcpp commented Sep 7, 2023

@df7cb -ffloat-store worked on my side on Debian 32 bit under Virtual Box. Please, make sure that you recompiled all files with this option. Could you please try my patch #62 ? It works on my side.

@df7cb
Copy link
Contributor Author

df7cb commented Sep 7, 2023

Your patch fixes the problem ✔️

@vitcpp vitcpp closed this as completed in #62 Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants