Skip to content
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

LineString.IsClosed() does not use DoubleComparer for Position #137

Open
jnyrup opened this issue Nov 29, 2019 · 1 comment
Open

LineString.IsClosed() does not use DoubleComparer for Position #137

jnyrup opened this issue Nov 29, 2019 · 1 comment

Comments

@jnyrup
Copy link
Contributor

jnyrup commented Nov 29, 2019

Two instances of Position are considered equal when their Latitude, Longitude and Altitude are approximately equal using a DoubleComparer.

LineString.IsClosed considers an instance to be closed if Latitude, Longitude and Altitude of its first and last IPosition coordinate are exactly equals.

This seems to be potential inconsistent when constructing a LineString with Position as IPosition.

Is that intended behavior?

@xfischer
Copy link
Member

Well it's a dilema here. I don't know what was the intended behavior when the project was initiated.
It may be for that reason : when LineStrings are not closed, this will break some systems (SQL Server is strict on that for instance).

But comparing until the 10th decimal digit (as DoubleComparer does) is fair enough to state it's the same coordinate. We are talking about micrometers here.

More over equality with double is something variable upon where the code is executed and strict equality should be replaced by difference is below double.Epsilon. So the DoubleComparer is a good choice here, as it checks for equality for acceptable ranges.

So I think we could remove this strict equality check and prefer the DoubleComparer.

We then may add candies for strict closeness such as EnsureIsClosed() which can replace last position by exactly the start position only if DoubleComparer states that they are equal.

If anyone watching this thinks it's a bad idea please comment down here.

And by the way, thanks for your implication @jnyrup !

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

No branches or pull requests

2 participants