Add IsValid(int idx) and IsClosed to CordinateSequence

[zgabi]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the existing coding patterns and practice as demonstrated in the repository.

• I have provided test coverage for my change (where applicable)

Description

During the validation of the LineString object some extra Coordiate objects are created when not the default CoordinateSequence is used. (For example a PackedCoordinateSequence or a custom sequence)
It would be doog if the CoordinateSequence could validate the Nth coordinate or check whether it is closed.
This feature request/PR adds 2 new virtual methods/property to the base CoordinateSequence class to check the validity of the Nth coordiante and whether the sequence is closed without getting the actual coordinate object.
It also adds an implementation for the PackedCoordinateSequence class.

[FObermaier]

I tend to support this enhancement.
I'm not sure about the naming of IsValidCoordinate(int), I think IsCoordinateValidAt(int) is slightly better.

Unit tests are missing

[FObermaier]

public sealed override bool IsValidCoordinate(int i)

[FObermaier]

dito

[zgabi]

IsValidCoordinate renamed to IsCoordinateValidAt.
IsCoordinateValidAt is sealed in the packed class.
And added unit test for the packed classes. The base class is covered by a lot of tests.

double.IsFinite is not available in netstandard2.0, however you have an aggressive inlined private method in the Coordinate class. Maybe this method could be moved to some helper class and use the same method from all locations.
Maybe to the Mathematics.MathUtils class or to the Utilities.Global class.

[zgabi]

Why is this try-catch here?

NetTopologySuite/test/NetTopologySuite.Tests.NUnit/Geometries/Implementation/PackedCoordinateSequenceTest.cs

Lines 203 to 210 in b2897e0

	try
	{
	var seq = factory.Create(5, 2, 1);
	Assert.Fail("Dimension=2/Measure=1 (XM) not supported");
	}
	catch (ArgumentException)
	{
	}

I accicdentally copied this method for the new test methods... Now I removed the try-catches from my new test methods.

[FObermaier]

double.IsFinite is not available in netstandard2.0, however you have an aggressive inlined private method in the Coordinate class. Maybe this method could be moved to some helper class and use the same method from all locations. Maybe to the Mathematics.MathUtils class or to the Utilities.Global class.

I propose to add

/// <summary>
/// Tests if a value is a valid for an ordinate.
/// Valid ordinate values are finite.
/// </summary>
/// <param name="value">The ordinate value</param>
/// <returns><c>true</c> if <paramref name="value"/> has a finite value</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool IsValidOrdinateValue(double value)
{
#if NETSTANDARD2_1_OR_GREATER
    return double.IsFinite(value);
#else
    return (~BitConverter.DoubleToInt64Bits(value) & 0x7ff0000000000000L) != 0;
#endif
}

to either Coordinate or Coordinates-utility class

[airbreather]

double.IsFinite is not available in netstandard2.0, however you have an aggressive inlined private method in the Coordinate class. Maybe this method could be moved to some helper class and use the same method from all locations. Maybe to the Mathematics.MathUtils class or to the Utilities.Global class.

I propose to add

/// <summary>
/// Tests if a value is a valid for an ordinate.
/// Valid ordinate values are finite.
/// </summary>
/// <param name="value">The ordinate value</param>
/// <returns><c>true</c> if <paramref name="value"/> has a finite value</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool IsValidOrdinateValue(double value)
{
#if NETSTANDARD2_1_OR_GREATER
    return double.IsFinite(value);
#else
    return (~BitConverter.DoubleToInt64Bits(value) & 0x7ff0000000000000L) != 0;
#endif
}

to either Coordinate or Coordinates-utility class

Having IsValidOrdinateValue sounds fine to me, but I'd just use the BitConverter version and not bother with the NETSTANDARD2_1_OR_GREATER check. If anything, based on a small benchmark, the BitConverter version appears like it might be faster than calling double.IsFinite directly (at least on my machine with net8.0).

[zgabi]

Ok, I added the IsValidOrdinateValue method without conditional compiling.

[FObermaier]

Thanks for finding the other places where ordinate validity plays a role!

[FObermaier]

I've thought a bit about IsClosed implemented on CoordinateSequence:
We have the IsClosed property on LineString and LinearRing. Both have different semantics. For empty LinearRings this leads to the situation that ring.IsClosed != ring.CoordinateSequence.IsClosed which is unfortunate.

To prevent the creation of Coordinate instances for the IsClosed test, I instead suggest to open visibility of CoordinateSequences.IsEqualAt(CoordinateSequence seq1, int pos1, CoordinateSequence seq2, int pos2, int numSpatial, int numMeasures) to internal and use that.

[zgabi]

Yes, emply LineString (base class) is not closed. Empty LineRing is closed. But it was in this way earlier.
The implentation of the CoordinateSequence.IsClosed is the same as the previous LineString implementation.

What do you suggest?

I don't know whether it is important or not that the empty LineString is not closed. Maybe it could be also closed?
I run the tests with closed empty LineStrings. The result is 3 failing test:
TestIsClosed
testCreateEmptyGeometry
testEmptyLineString

2 of them are speifically checking the IsClosed value, so probably they are not relevant. The check in the test should be negated.
The testCreateEmptyGeometry is interesting. It is checking the BoundaryDimension of the empty LineString. Expeted value is 0. But after the change it is -1.

[zgabi]

Yes, it could use the CoordinateSequences.IsEqualAt with numSpatial=2 and numMeasures=0. However it is more complex, so probably slightly slower than checking simply the X and Y properties (or GetX and GetY results in the packed sequence).
Since it has a loop, gets the X and Y values with GetOrdinate which using a switch (e.g ordinate==1 returns X).

How about putting an IsEqualAt2D to the CoordinateSequences, which checks only the X and Y coorinates and no measures? (Similar to the Coordiante.Equals2D method)

[zgabi]

What about this PR? Should I change anything else?
Currently the IsClosed value in this PR equivalent with the current "develop" code. So could you please merge it?
The IsClosed can be changed in a separate task/PR if you agree on how to change it.