measures regression test fails in 1.5 branch

[strk]

*** measures_expected   Wed Mar 28 10:24:51 2012
--- /tmp/pgis_reg_14991/test_25_out     Wed Mar 28 10:28:44 2012
***************
*** 1,4 ****
! 113|291
  114|140
  115|140
  116|4.24264068711929
--- 1,4 ----
! 113|297.5
  114|140
  115|140
  116|4.24264068711929
***************
*** 8,14 ****
  135|13
  136|13
  dist|1|1
! 113|291
  114|140
  115|140
  116|4.24264068711929
--- 8,14 ----
  135|13
  136|13
  dist|1|1
! 113|297.5
  114|140
  115|140
  116|4.24264068711929

[strk]

This was with PostgreSQL 9.1 on an x86_64 GNU/Linux and r9274

[nicklas]

select '113', 
area2d('MULTIPOLYGON( ((0 0, 10 0, 10 10, 0 10, 0 0)),
( (0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 7 5, 7 7 , 5 7, 5 5) ) 
,( (0 0, 10 0, 10 10, 0 10, 0 0),(5 5, 7 5, 7 7, 5 7, 5 5),
(1 1,2 1, 2 2, 1 2, 1 1)     ) )'::GEOMETRY) as value;

gives 291 (as expected) to me on 32 bit linux.

[nicklas]

The only change in area calculation I know of is the one from ticket: http://trac.osgeo.org/postgis/ticket/810

[nicklas]

I do get the same wrong answer at 64 bit (297.5)

It seems like r9273 is the problem. It works as expected on r9272.

But I double checked and 32 bit seems to work as expected also with the change in r9273.

I don't see what in that change that can give different answers in 32-bit and 64-bit.

Sandro, you mention that r9273 removes a compiler warning, can that be a clue?

[strk]

Uhm, that commit does indeed look suspicious, in taht it copies P2 to P1 but _after_ P2 was modified !

[nicklas]

But P2 is just a local variable, why is that dangerous?

/Nicklas

[nicklas]

You are right that the problem is there.

If I revert just that part of the commit it works as expected.

The same error appears with this simplier polygon:

select 
area2d('POLYGON((5 5, 7 5, 7 7 , 5 7, 5 5) ) '::GEOMETRY)
 as value;

[nicklas]

Oh, sorry, now I see it.

You are of course right.

p1 gets modified twice, first as p2 and then as p1.

[strk]

right! Sorry about that. We should check if the same problem is in trunk too. I think first thing is adding a test making this clear, and possibly breaking both 32 and 64bit systems. Probably any polygon not starting with 0,0 would do. The bigger numbers to clearer.

[nicklas]

I have done some testing by putting notices in the function and I see how p1 gets changed in a wrong way.

I have removed any modification of p1 and set p1.x and p1.y to 0.0 before the iteration. Then it seems to work as expected both on 23 and 64 bit.

Do you think we should investigate ebtter the difference between 23 and 64 bit or should I just commit?

Little strange that it workwed at all before since P1 did never get any value before the first iteration.

[nicklas]

well, my 23-bit system is unique, I know

[strk]

Did you handle to produce a testcase giving wrong answers on 64bit at all ? If it's a real conceptual bug we should aim for that. If it's just a numerical robustness thing we should instead tolerate better resolution on the 64bit system.

[nicklas]

You mean finding a test case for 32 bit that gives wrong answer?

I will look at that. It should be quite easy to find.

I'll get back

[nicklas]

There was the same error all the time in 32-bit. I was testing a 9.0 version but the fresh compile installed against 9.1.

So, I think it is quite obvious what is happening and I think the attached patch fixes it. If there is no objections I will commit it and back port to 1.4

But looking at the trunk I don't understand what is happening. It behaves correctly, but is at least hard to read.

I will look at it and try to understand. If it is not clear to others what is happening I would like to rewrite it more along the line of 1.5.

[nicklas]

ok, I read in #810 that it is a totally new algorithm applied. I will not touch the trunk

[strk]

I confirm the 32-bit system also fails ! The patch looks fine to me.

Don't forget to update the NEWS file iff r9273 was in 1.5.3

[nicklas]

fixed in 9571

Trunk and 1.4 is not affected.

This was an unreleased bug so no NEWS