Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> I'm a bit confused how this patch has gone through several revisions
> since, but has been marked as "ready for committer" since 2017-09-05. Am
> I missing something?

This is floating between commitfests for longer than a year.
Aleksander Alekseev set it to "ready for committer", but Kyotaro
HORIGUCHI continues his review.  I hope not many issues have remained.

Re: [HACKERS] [PATCH] Improve geometric types

[Andres Freund]

Hi,

On 2018-02-07 16:46:38 +0100, Emre Hasegeli wrote:
> I am incorporating the fix you have posted to the other thread to this patch.

You've not posted a version fixing the issues in the surrounding thread,
do I see that right?

I'm a bit confused how this patch has gone through several revisions
since, but has been marked as "ready for committer" since 2017-09-05. Am
I missing something?

Greetings,

Andres Freund

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

>  - line_eq looks too complex in the normal (not containing NANs)
>cases.  We should avoid such complexity if possible.
>
>One problem here is that comparison conceals NANness of
>operands. Conversely arithmetics propagate it. We can converge
>NANness into a number. The attached line_eq() doesn that. This
>doesn't have almost no additional complexity when NAN is
>involved. I believe it qbehaves in the same way
>and shares a doubious behavior like this.
>
>=# select '{nan, 1, nan}'::line = '{nan, 2, nan}'::line;
> ?column?
>--
> t
>
>But probably no point in fixing(?) it.

I think we should fix it.

>The attached file contains line_eq, point_eq_point and
>circle_same. I expect that line_eq is fast but other two are
>doubious.

I haven't got an attachment.

>. . . Mmm.. The function seems broken. I posted the fix for
>the existing version is posted, and line_perp() in the attched
>file will work fine.

I am incorporating the fix you have posted to the other thread to this patch.

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

Hello,

At Wed, 31 Jan 2018 17:33:42 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20180131.173342.26333067.horiguchi.kyot...@lab.ntt.co.jp>
> 0003: This patch replaces "double" with float and bare arithmetic
> and comparisons on double to special functions accompanied
> with checking against abnormal values.
> 
>  - Almost all of them are eliminated with a few safe exceptions.
> 
> - circle_recv(), circle_distance(), dist_pc(), dist_cpoint()
>   are using "< 0.0" comparison but it looks fine.
> 
> - pg_hypot is right to use bare arithmetics.
> 
> ! circle_contain_pt() does the following comparison and it
>   seems to be out of our current policy.
> 
>   point_dt(center, point) <= radius
> 
>   I suppose this should use FPle.
> 
>   FPle(point_dt(center, point), radius)
> 
>   The same is true of circle_contain_pt(), pt_contained_circle .

 - line_eq looks too complex in the normal (not containing NANs)
   cases.  We should avoid such complexity if possible.

   One problem here is that comparison conceals NANness of
   operands. Conversely arithmetics propagate it. We can converge
   NANness into a number. The attached line_eq() doesn that. This
   doesn't have almost no additional complexity when NAN is
   involved. I believe it qbehaves in the same way
   and shares a doubious behavior like this.

   =# select '{nan, 1, nan}'::line = '{nan, 2, nan}'::line;
?column? 
   --
t

   But probably no point in fixing(?) it.

   The attached file contains line_eq, point_eq_point and
   circle_same. I expect that line_eq is fast but other two are
   doubious.

0004:

 - line_perp

   We can detect perpendicularity without division.

   The normal vecotor of Ax + Bx + C = 0 is (A, B). If two lines
   are perpendicular, the inner product of the normal vectors of
   v1 and v2 is 0. No point in dividing.

   l1->A * l2->A + l1->B * l2->B == 0

   . . . Mmm.. The function seems broken. I posted the fix for
   the existing version is posted, and line_perp() in the attched
   file will work fine.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> ! circle_contain_pt() does the following comparison and it
>   seems to be out of our current policy.
>
>   point_dt(center, point) <= radius
>
>   I suppose this should use FPle.
>
>   FPle(point_dt(center, point), radius)
>
>   The same is true of circle_contain_pt(), pt_contained_circle .

box_contain_point() also doesn't use the macros.  They are certainly
inconsistent, but I don't think it would be an improvement to make
them use the macros.  As we have discussed, there are many problems
with the current application of EPSILON.  I think we would be better
off not using the macros for none of the containment operators, but
this is out of my scope for now.

> # Sorry, that' all for today..

I am waiting the rest of your review to post the new versions.

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> 1."COPT=-DGEODEBUG make" complains as follows.
>
>   | geo_ops.c:2445:62: error: invalid type argument of unary ‘*’ (have 
> ‘float8 {aka double}’)
>   |   printf("dist_ppoly_internal- segment 0/n distance is %f\n", *result);

Fixing.

> 2. line_construct_pm has been renamed to line_construct. I
>noticed that the patch adds the second block for "(m == 0.0)"
>(from the ealier versions) but it seems to work exactly as the
>same to the "else" block. We need a comment about the reason
>for the seemingly redundant second block.

It is indeed redundant.  I am removing it.

> 3. point_sl can return -0.0 and that is a thing that this patch
>intends to avoid. line_invsl has the same problem.

The existing version of the function has the same issue.  I think we
need a global solution for -0s.  See the float-zero patch.

> 4. lseg_interpt_line is doing as follows.
>
>>  if (FPeq(lseg->p[0].x, interpt.x) && FPeq(lseg->p[0].y, interpt.y))
>>*result = lseg->p[0];
>>  else if (FPeq(lseg->p[1].x, interpt.x) && FPeq(lseg->p[1].y, interpt.y))
>>*result = lseg->p[1];
>
>I suppose we can use point_pt_point for this purpose.

Yes, I am changing it.

>>  if (point_eq_point(>p[0], ))
>>*result = lseg->p[0];
>>  else if (point_eq_point(>p[1], ))
>>*result = lseg->p[1];
>
>However I'm not sure that adjusting the intersection to the
>tips of the segment is good or not. Adjusting onto the line
>can be better in another case. lseg_interpt_lseg, for
>instance, checks lseg_contain_point on the line parameter of
>lseg_interpt_line.

Me neither, but it is probably better than returning a point that
extends the endpoints of the segment.  I am inclined to leave it alone
for now.

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

At Wed, 31 Jan 2018 13:09:09 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20180131.130909.210233873.horiguchi.kyot...@lab.ntt.co.jp>
> At Sun, 21 Jan 2018 21:59:19 +0100, Emre Hasegeli  wrote 
> in 
> > New versions are attached including all changes we discussed.
> 
> Thanks for the new version.
> 
> # there's many changes from the previous version..
> 
> About 0001 and 0002.
> 
> 1."COPT=-DGEODEBUG make" complains as follows.
> 
>   | geo_ops.c:2445:62: error: invalid type argument of unary ‘*’ (have 
> ‘float8 {aka double}’)
>   |   printf("dist_ppoly_internal- segment 0/n distance is %f\n", *result);
> 
> 2. line_construct_pm has been renamed to line_construct. I
>noticed that the patch adds the second block for "(m == 0.0)"
>(from the ealier versions) but it seems to work exactly as the
>same to the "else" block. We need a comment about the reason
>for the seemingly redundant second block.
> 
> 3. point_sl can return -0.0 and that is a thing that this patch
>intends to avoid. line_invsl has the same problem.
> 
> 4. lseg_interpt_line is doing as follows.
> 
>>  if (FPeq(lseg->p[0].x, interpt.x) && FPeq(lseg->p[0].y, interpt.y))
>>*result = lseg->p[0];
>>  else if (FPeq(lseg->p[1].x, interpt.x) && FPeq(lseg->p[1].y, interpt.y))
>>*result = lseg->p[1];
> 
>I suppose we can use point_pt_point for this purpose.
> 
>>  if (point_eq_point(>p[0], ))
>>*result = lseg->p[0];
>>  else if (point_eq_point(>p[1], ))
>>*result = lseg->p[1];
> 
>However I'm not sure that adjusting the intersection to the
>tips of the segment is good or not. Adjusting onto the line
>can be better in another case. lseg_interpt_lseg, for
>instance, checks lseg_contain_point on the line parameter of
>lseg_interpt_line.
> 

> # I'll be back later..

I've been back.

0003: This patch replaces "double" with float and bare arithmetic
and comparisons on double to special functions accompanied
with checking against abnormal values.

 - Almost all of them are eliminated with a few safe exceptions.

- circle_recv(), circle_distance(), dist_pc(), dist_cpoint()
  are using "< 0.0" comparison but it looks fine.

- pg_hypot is right to use bare arithmetics.

! circle_contain_pt() does the following comparison and it
  seems to be out of our current policy.

  point_dt(center, point) <= radius

  I suppose this should use FPle.

  FPle(point_dt(center, point), radius)

  The same is true of circle_contain_pt(), pt_contained_circle .



# Sorry, that' all for today..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

At Sun, 21 Jan 2018 21:59:19 +0100, Emre Hasegeli  wrote in 

> New versions are attached including all changes we discussed.

Thanks for the new version.

# there's many changes from the previous version..

About 0001 and 0002.

1."COPT=-DGEODEBUG make" complains as follows.

  | geo_ops.c:2445:62: error: invalid type argument of unary ‘*’ (have ‘float8 
{aka double}’)
  |   printf("dist_ppoly_internal- segment 0/n distance is %f\n", *result);

2. line_construct_pm has been renamed to line_construct. I
   noticed that the patch adds the second block for "(m == 0.0)"
   (from the ealier versions) but it seems to work exactly as the
   same to the "else" block. We need a comment about the reason
   for the seemingly redundant second block.

3. point_sl can return -0.0 and that is a thing that this patch
   intends to avoid. line_invsl has the same problem.

4. lseg_interpt_line is doing as follows.

   >  if (FPeq(lseg->p[0].x, interpt.x) && FPeq(lseg->p[0].y, interpt.y))
   >*result = lseg->p[0];
   >  else if (FPeq(lseg->p[1].x, interpt.x) && FPeq(lseg->p[1].y, interpt.y))
   >*result = lseg->p[1];

   I suppose we can use point_pt_point for this purpose.

   >  if (point_eq_point(>p[0], ))
   >*result = lseg->p[0];
   >  else if (point_eq_point(>p[1], ))
   >*result = lseg->p[1];

   However I'm not sure that adjusting the intersection to the
   tips of the segment is good or not. Adjusting onto the line
   can be better in another case. lseg_interpt_lseg, for
   instance, checks lseg_contain_point on the line parameter of
   lseg_interpt_line.

# I'll be back later..

regards

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> I'm fine with the current shape. Thanks. Maybe the same
> discussion applies to polygons. (cf. poly_overlap)

It indeed does.  I am incorporating.

>  line_closept_point asserts line_interpt_line returns true but it
>  is hazardous. It is safer if line_closept_point returns NaN if
>  line_interpt_line returns false.

Yes, it makes more sense.  I am changing it.

>> >  I understand that we don't need to consider NAN is equal to NAN.
>> >  circle_same, point_eq_point, line_eq no longer needs such
>> >  change?
>>
>> I though it would be better to consider NaNs equal only on the
>> equality operators.  That is why only they are changed that way.
>
> I'm convinced by that.

Great.  I am documenting this decision better on the patches.

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

Hello,

At Thu, 18 Jan 2018 16:01:01 +0100, Emre Hasegeli  wrote in 

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> 0001:
>
> - You removed the check of parallelity check of
>   line_interpt_line(old line_interpt_internal) but line_parallel
>   is not changed so the consistency between the two functions are
>   lost. It is better to be another patch file (maybe 0004?).

I am making line_parallel() use line_interpt_line().  What they do is
exactly the same.

> - + Assert(p1->npts > 0 && p2->npts > 0);
>
>   Other path_ functions are allowing path with no points so just
>   returning false if (p1->npts == 0 || p2->npts == 0) seems
>   enough. Assertion should be used only for something server
>   cannot continue working. (division by zero doesn't crash
>   server) I saw other similar assertions in the patch.

path_in() and path_recv() reject paths with no points.  I thought we
shouldn't spend cycles for things that cannot happen.  I can revert
this part if you find previous practice better.

> 0003:
>
>  close_pl/ps/lseg/pb/ls/sb have changed to return null when
>  lseg_closept_line returns NAN, but they are defined as strict
>  and that is reasonable. Anyway pg_hypot returns NaN only when
>  parameters contain INF or NAN so we should error out when it
>  returns nan.

I though strict is only related to the input being NULL.  Tom Lane
made close_ps() return NULL with commit
278148907a971ec7fa4ffb24248103d8012155d2.  The other functions
currently fail with elog()s from DirectFunctionCalls.  I don't have
strong preference for NULL or an error.  Could you please suggest an
errcode and errmsg, if you have?

>  circle_in rejects negative radius and circle_recv modived to
>  reject zero and negative. What is the reason for the change?

Overlooking.  Thank you for noticing.  I am fixing it.

>  I understand that we don't need to consider NAN is equal to NAN.
>  circle_same, point_eq_point, line_eq no longer needs such
>  change?

I though it would be better to consider NaNs equal only on the
equality operators.  That is why only they are changed that way.

>  Assertion is added in pg_hypot but it is impossible for result
>  to be negative there. Why do you added it?

True.  I am removing it.

> 0004:
>
>  Looking line_recv's change, I became to think that FPxx macros
>  is provided for coordinate values. I don't think it is applied
>  to non-coordinate values like coefficients. If some kind of
>  tolerance is needed here, it is not the same with the EPSILON.
>
>  +if (FPzero(line->A) && FPzero(line->B))
>  +ereport(ERROR,
>
>  So, the above should be exact comparison with 0.0 and line_in
>  also should be the same. And maybe the same thing should be done
>  at many places..

I agree you.  EPSILON is currently applied prematurely.  I am trying
to stay away from EPSION related issues to improve the chances to get
this part committed.

>  But the following line of line_parallel still requires some kind
>  of tolerance to work properly. Since this patch is an
>  imoprovement patch, I think we can change it to the vector method.

I am making line_parallel() use line_interpt_line() in response to
your first remark.  Vector based algorithm is probably better for
every use of line_interpt_line(), but it is a bigger change with more
user visible effects.

>  The problem of line_distance is an existing one so we can leave
>  it alone. It returns 0.0 for the most cases but it is a
>  long-standing behavior.. (Anyway I don't find a reasonable
>  definition of the distance between very-nearly parallel lines..)

Exact comparison with 0.0 instead of FPzero() would also be an
improvement for line_distance(), but I am not doing it now.

> -- Sorry time's up today.

I am waiting for the rest of your review to post the new versions.

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

Hello,

I played more around geometric types and recalled that geometric
types don't have orderings. Also have corrected some other
misunderstanding. Sorry for my confusion and I think I returned
on the way.

At Wed, 17 Jan 2018 18:59:30 +0100, Emre Hasegeli  wrote in 

> > I'm not sure what you mean by the "basic comparison ops"  but I'm
> > fine with the policy, handling each component values in the same
> > way with float. So point_eq_point() is right and other functions
> > should follow the same policy.
> 
> I mean <, >, <= and >= by basic comparison operators.  Operators with
> those symbols are available for some geometric types, but they are
> comparing the sizes of the objects.  Currently only the equality
> operators follow the same policy with point_eq_point (), others never
> return true when NaNs are involved.
> 
> > Sorry for going back and force, but I don't see a difference
> > between it and the original behavior from the point of view of
> > reasonability. Isn't is enough to let each component comparison
> > follow float by redefining FPxx() functions?
> 
> My previous patch was doing exactly that.  I though that is not what
> we want to do.  Do we want box_overlap() to return true when NaNs are
> involved?

All comparison operators don't need consideration on
ordering. And the existing comparison operators behaves
diferrently from float and already behaves in the way of bare
float. There's no behavior as the whole of an object. I have
fixed my understanding as this. (And I saw all patches altoghter
by mistake..) As the result most my concern was pointless.


0001:

- You removed the check of parallelity check of
  line_interpt_line(old line_interpt_internal) but line_parallel
  is not changed so the consistency between the two functions are
  lost. It is better to be another patch file (maybe 0004?).

- + Assert(p1->npts > 0 && p2->npts > 0);

  Other path_ functions are allowing path with no points so just
  returning false if (p1->npts == 0 || p2->npts == 0) seems
  enough. Assertion should be used only for something server
  cannot continue working. (division by zero doesn't crash
  server) I saw other similar assertions in the patch.


0002: I have no comment so far on this patch.

0003: 

 close_pl/ps/lseg/pb/ls/sb have changed to return null when
 lseg_closept_line returns NAN, but they are defined as strict
 and that is reasonable. Anyway pg_hypot returns NaN only when
 parameters contain INF or NAN so we should error out when it
 returns nan.

 circle_in rejects negative radius and circle_recv modived to
 reject zero and negative. What is the reason for the change?

 I understand that we don't need to consider NAN is equal to NAN.
 circle_same, point_eq_point, line_eq no longer needs such
 change?

 Assertion is added in pg_hypot but it is impossible for result
 to be negative there. Why do you added it?

0004:
 
 Looking line_recv's change, I became to think that FPxx macros
 is provided for coordinate values. I don't think it is applied
 to non-coordinate values like coefficients. If some kind of
 tolerance is needed here, it is not the same with the EPSILON.

 +if (FPzero(line->A) && FPzero(line->B))
 +ereport(ERROR,

 So, the above should be exact comparison with 0.0 and line_in
 also should be the same. And maybe the same thing should be done
 at many places..

 But the following line of line_parallel still requires some kind
 of tolerance to work properly. Since this patch is an
 imoprovement patch, I think we can change it to the vector method.

 The problem of line_distance is an existing one so we can leave
 it alone. It returns 0.0 for the most cases but it is a
 long-standing behavior.. (Anyway I don't find a reasonable
 definition of the distance between very-nearly parallel lines..)


-- Sorry time's up today.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> I'm not sure what you mean by the "basic comparison ops"  but I'm
> fine with the policy, handling each component values in the same
> way with float. So point_eq_point() is right and other functions
> should follow the same policy.

I mean <, >, <= and >= by basic comparison operators.  Operators with
those symbols are available for some geometric types, but they are
comparing the sizes of the objects.  Currently only the equality
operators follow the same policy with point_eq_point (), others never
return true when NaNs are involved.

> Sorry for going back and force, but I don't see a difference
> between it and the original behavior from the point of view of
> reasonability. Isn't is enough to let each component comparison
> follow float by redefining FPxx() functions?

My previous patch was doing exactly that.  I though that is not what
we want to do.  Do we want box_overlap() to return true when NaNs are
involved?

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

Hello,

I'm still wandering on the way and confused. Sorry for
inconsistent comments in advanceX-(

At Sun, 14 Jan 2018 13:20:57 +0100, Emre Hasegeli  wrote in 

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> I found seemingly inconsistent handling of NaN.
>
> - Old box_same assumed NaN != NaN, but new assumes NaN ==
>   NaN. I'm not sure how the difference is significant out of the
>   context of sorting, though...

There is no box != box operator.  If there was one, previous code
would return false for every input including NaN, because it was
ignorant about NaNs other than a few bandaids to avoid crashes.

My idea is to make the equality (same) operators behave like the float
datatypes treating NaNs as equals.  The float datatypes also treat
NaNs as greater than any non-NAN.  We don't need to worry about this
part now, because there are no basic comparison operators defined for
the geometric types.

> - box_overlap()'s behavior has not been changed. As the result
>   box_same and box_overlap now behaves differently concerning
>   NaN handling.

After your previous email, I though this would be the right thing to
do.  I made the containment and placement operators return false for
NaN input unless we can find the result using non-NaN coordinates.  Do
you find this behaviour reasonable?

> - line_construct_pts has been changed so that generates
>   {NaN,-1,NaN} for two identical points (old code generated
>   {-1,0,0}) but I think the right behavior here is erroring out.
>   (as line_in behaves.)

I agree.  We should also check for the endpoints being the same on
lseg_interpt functions.  I will make the changes on the next version.

> I feel that it'd better to define how to handle non-numbers
> before refactoring.  I prefer to just denying NaN as a part of
> the geometric types, or any input containing NaN just yields a
> result filled with NaNs.

It would be nice to deny NaNs altogether, but I don't think it is
feasible.  People cannot restore their existing data if we start doing
that.  It would also require us to check any result we emit being NaN
and error out in more cases.

> The next point is reasonability of the algorithm and consistency
> among related functions.
>
> - line_parallel considers the EPSILON(ugaa) with different scale
>   from the old code, but both don't seem reasonable.. It might
>   not be the time to touch it, but if we changed the behavior,
>   I'd like to choose reasonable one. At least the tolerance of
>   parallelity should be taken by rotation, not by slope.

I think you are right.  Vector based algorithms would be good for many
other operations as well.  This would be more changes, though.  I am
try to reduce the amount of changes to increase my chances to get this
committed.  I just used slope() there to increase the code reuse and
to make the operation symmetrical.  I can revert it back to its
previous state, if you thing it is better.

> So we might should calculate the distance in different (not
> purely mathematical/geometrical) way. For example, if we can
> assume the geometric objects are placed mainly around the origin,
> we could take the distance between the points nearest to origin
> on both lines. (In other words, between the foots of
> perpendicular lines from the origin onto the both lines). Of
> couse this is not ideal but...

The EPSILON is unfair to coordinates closer to the origin.  I am not
sure what it the best way to improve this.  I am trying to avoid
dealing with EPSILON related issues in the scope of these changes.

> At last, just a simple comment.
>
> - point_eq_point() assumes that NaN == NaN. This is an inherited
>   behavior from old float[48]_cmp_internal() but it's not a
>   common treat. point_eq_point() needs any comment about the
>   special definition as float[48]_cmp_internal has.

I hope I answered this on the previous questions.  I will incorporate
the decision to threat NaNs as equals into the comments and the commit
messages on the next version, if we agree on it.

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

Hello, sorry for the absense.

(Sorry for the long but should be hard-to-read article..)

At Thu, 4 Jan 2018 14:53:47 +0100, Emre Hasegeli  wrote in 

> Rebased versions are attached.

Thank you for the new patch.

I'm looking 0001 patch. This does many things at once but seems
hard to split into step-by-step patches. So I reviewed this from
the viewpoint that how each of the external function is
changed. (Attatchment 1).

I think that this patch is not intending to change any behaviors
of the external functions, but intending make it mathematically
reasonable. So some behavioral changes are ineviablly
requried. The problem here is what is the most reasonable (and
acceptable) behavior.

The comments below are just the starting of a discussion about
some aspects of design.  I'm sorry that this discussion might be
changing the way to go largily, but I'd like to hear what you
think about two topics.


I found seemingly inconsistent handling of NaN.

- Old box_same assumed NaN != NaN, but new assumes NaN ==
  NaN. I'm not sure how the difference is significant out of the
  context of sorting, though...

- box_overlap()'s behavior has not been changed. As the result
  box_same and box_overlap now behaves differently concerning
  NaN handling.

- line_construct_pts has been changed so that generates
  {NaN,-1,NaN} for two identical points (old code generated
  {-1,0,0}) but I think the right behavior here is erroring out.
  (as line_in behaves.)


I feel that it'd better to define how to handle non-numbers
before refactoring.  I prefer to just denying NaN as a part of
the geometric types, or any input containing NaN just yields a
result filled with NaNs.


The next point is reasonability of the algorithm and consistency
among related functions.

- line_parallel considers the EPSILON(ugaa) with different scale
  from the old code, but both don't seem reasonable.. It might
  not be the time to touch it, but if we changed the behavior,
  I'd like to choose reasonable one. At least the tolerance of
  parallelity should be taken by rotation, not by slope.

I think that one reasonable way to do this is taking the distance
between the far ends of two direction (unit) vectors.
  
Assuming Ax + By + C = 0, the direction vecotr dv(x, y) for the
line is:

n = sqrt(A^2 + B^2), dv = (B / n, -A / n).

  (and the normal vector nv = (A / n, B / n))

The vector needs to be restricted within upper or any two
successive quadrants so that it is usable for this objective. So
let's restrict A to be negative value for now. Something like the
follwoing would be an answer.

  void line_direction_vector(Point *result, LINE *l)
  {
float8 A = l->A;
float8 B = l->B;
float8 n;

n = HYPOT(A, B);

/* keep the result vector within the 1st-2nd quadrants */
if (A > 0)
{
   A = -A;
   B = -B;
}
point_construct(result, B / n, -A / n);
  }

  bool line_parallel(LINE *l1, LINE *l2)
  {
Point d1, d2;

line_direction_vector(, l1);
line_direction_vector(, l2);
return FPzero(point_dt(, ));
  }
  
Also perpendicularity is detected by comparing the direction
vector of one line and the normal vector of another in the same
manner.

  void line_normal_vector(Point *result, LINE *l)
  {
float8 A = l->A;
float8 B = l->B;
float8 n;

/* keep the result vector within the 1st-2nd quadrants */
n = HYPOT(A, B);
if (B < 0)
{
   A = -A;
   B = -B;
}
point_construct(result, A / n, B / n);
  }

  bool line_perp(LINE *l1, LINE *l2)
  {
Point d1, d2;

line_direction_vector(, l1);
line_normal_vector(, l2);
return FPzero(point_dt(, ));
  }
  
In relation to this, equality of two lines is also nuisance.
>From the definitions, two equal lines are parallel. In
contraposition, two nonparallel lines cannot be equal. This
relationship is represented by the following expression:

 is_equal =: line_parallel(l1, l2) && FPzero(line_distance(l1, l2))

But the line_distance returns zero in most cases even if it is
considered "parallel" with considering tolerance. This means that
There's almost no two lines that are parallel but not equal (that
is, the truely parallel lines)... sigh.

So we might should calculate the distance in different (not
purely mathematical/geometrical) way. For example, if we can
assume the geometric objects are placed mainly around the origin,
we could take the distance between the points nearest to origin
on both lines. (In other words, between the foots of
perpendicular lines from the origin onto the both lines). Of
couse this is not ideal but...

The same discussion also affects line_interpt_line or other
similar functions.


At last, just a simple comment.

- point_eq_point() assumes that NaN == NaN. This is an inherited
  behavior from old float[48]_cmp_internal() but it's not a
  common treat. point_eq_point() needs any comment about the
  special 

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> Does this patch series fix bug #14971?

No, it doesn't.  I am trying to improve things without touching the EPSILON.

Re: [HACKERS] [PATCH] Improve geometric types

[Alvaro Herrera]

Does this patch series fix bug #14971?
https://www.postgresql.org/message-id/20171213172806.20142.73...@wrigleys.postgresql.org

Eric: see
https://www.postgresql.org/message-id/CAE2gYzzvp=uvpw4fytoaevyk-wze4jw7u2s1glrok35mr1-...@mail.gmail.com
for last version of patch.

Motivation for patch is at
https://www.postgresql.org/message-id/CAE2gYzxF7-5djV6-cEvqQu-fNsnt%3DEqbOURx7ZDg%2BVv6ZMTWbg%40mail.gmail.com

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [PATCH] Improve geometric types

[Michael Paquier]

On Thu, Nov 30, 2017 at 3:28 AM, Emre Hasegeli  wrote:
> It would at least be dump-and-restore hazard if we don't let them in.
> The new version allows NaNs.
>
>> This gives a wrong result for NaN-containing objects.
>
> I removed the NaN aware comparisons from FP macros, and carefully
> reviewed the places that needs to be NaN aware.
>
> I am sorry that it took so long for me to post the new versions.  The
> more I get into this the more problems I find.  The new versions
> include non-trivial changes.  I would be glad if you can look into
> them.

I have moved this patch to next CF, with "needs review" as status as a
new version has been posted.
-- 
Michael

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> dist_pl is changed to take the smaller distance of both ends of
> the segment. It seems absorbing error, so it might be better
> taking the mean of the two distances. If you have a firm reason
> for the change, it is better to be written there, or it might be
> better left alone.

I am sorry for not paying enough attention to this before.  The
distance functions are meant to return the minimum distance.  Getting
the maximum is just wrong in here.  Can you think of any particular
error it would absorb?  Otherwise I will put this change back to the
patch for fixes.