Re: line_perp() (?-|) is broken.

[Kyotaro HORIGUCHI]

Hello, I'm returning to here sonn.

I regard Emre's work is aiming to refactor (not improve its
functionality of) the code, on the other hand thins one is a
separate bug fix which also should be backpatched.

But, honestrly I'm not sure such small fix would improve the
current situnation of the geometric operators at any rate.

At Sat, 03 Mar 2018 10:43:26 -0500, Tom Lane  wrote in 
<31399.1520091...@sss.pgh.pa.us>
> Emre Hasegeli  writes:
> >> Possibly this objection is pointless, because I'm not at all sure that
> >> the existing code is careful about how it uses FPeq/FPzero; perhaps
> >> we're applying EPSILON to all manner of numbers that don't have units
> >> of the coordinate space.  But this won't help make it better.

Agreed.

> > The existing code was probably paying attention to this particular
> > one, but it fails to apply EPSILON meaningfully to many other places.
> > For example lseg_parallel() and lseg_perp() applies it 2 times to the
> > same input.  First point_sl() compares x coordinates with FPeq(), and
> > then the returned slopes are compared again with FPeq().
> 
> Yeah, comparing a slope to EPSILON sure feels wrong :-(

It is a quite significant problem to fix and would be
controversial in detail. But, anyway, we are focusing on other
points that are less controversial. Then cook the EPSILON in the
next stage.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: line_perp() (?-|) is broken.

[Tom Lane]

Emre Hasegeli  writes:
>> Possibly this objection is pointless, because I'm not at all sure that
>> the existing code is careful about how it uses FPeq/FPzero; perhaps
>> we're applying EPSILON to all manner of numbers that don't have units
>> of the coordinate space.  But this won't help make it better.

> The existing code was probably paying attention to this particular
> one, but it fails to apply EPSILON meaningfully to many other places.
> For example lseg_parallel() and lseg_perp() applies it 2 times to the
> same input.  First point_sl() compares x coordinates with FPeq(), and
> then the returned slopes are compared again with FPeq().

Yeah, comparing a slope to EPSILON sure feels wrong :-(

regards, tom lane

Re: line_perp() (?-|) is broken.

[Emre Hasegeli]

> However, for either patch, I'm a bit concerned about using FPzero()
> on the inner product result.  To the extent that the EPSILON bound
> has any useful meaning at all, it needs to mean a maximum difference
> between two coordinate values.  The inner product has units of coordinates
> squared, so it seems like EPSILON isn't an appropriate threshold there.

In this case, I suggest this:

>if (FPzero(l1->A))
>PG_RETURN_BOOL(FPzero(l2->B));
>if (FPzero(l2->A))
>PG_RETURN_BOOL(FPzero(l1->B));
>if (FPzero(l1->B))
>PG_RETURN_BOOL(FPzero(l2->A));
>if (FPzero(l2->B))
>PG_RETURN_BOOL(FPzero(l1->A));
>
>PG_RETURN_BOOL(FPeq((l1->A * l2->A) / (l1->B * l2->B), -1.0));

> Possibly this objection is pointless, because I'm not at all sure that
> the existing code is careful about how it uses FPeq/FPzero; perhaps
> we're applying EPSILON to all manner of numbers that don't have units
> of the coordinate space.  But this won't help make it better.

The existing code was probably paying attention to this particular
one, but it fails to apply EPSILON meaningfully to many other places.
For example lseg_parallel() and lseg_perp() applies it 2 times to the
same input.  First point_sl() compares x coordinates with FPeq(), and
then the returned slopes are compared again with FPeq().

Re: line_perp() (?-|) is broken.

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> I happend to see a strange geometric calcualtion on master/HEAD.
> ...
> Instead, calculating inner product of the two direction vectors
> works as expected.
>   (l1->A * l2->A) + (l1->B * l2->B) == 0

This seems to be a strict subset of the changes in Emre Hasgeli's
latest geometric-types patch.  I'm disinclined to commit it unless
that patch gets rejected, as it'll just require Emre to rebase again.

However, for either patch, I'm a bit concerned about using FPzero()
on the inner product result.  To the extent that the EPSILON bound
has any useful meaning at all, it needs to mean a maximum difference
between two coordinate values.  The inner product has units of coordinates
squared, so it seems like EPSILON isn't an appropriate threshold there.

Possibly this objection is pointless, because I'm not at all sure that
the existing code is careful about how it uses FPeq/FPzero; perhaps
we're applying EPSILON to all manner of numbers that don't have units
of the coordinate space.  But this won't help make it better.

regards, tom lane