locationtech / jts

The JTS Topology Suite is a Java library for creating and manipulating vector geometry.
Other
1.93k stars 441 forks source link

JTS 1.14 Regression: CoordinateArraySequenceFactory handling of dimension 1 #135

Open jodygarnett opened 7 years ago

jodygarnett commented 7 years ago

Working on updating GeoTools to use JTS 1.14 (see https://github.com/geotools/geotools/pull/1665) and have run into a regression in the following:

    double[] ord = new double[] { x };
    int dim = 1;
    CoordinateSequence cs = csFact.create(n, dim);

This should produce a coodinate sequence with dimension 1, but it produces a coordinate sequence with dimension 3.

The JTS 1.14 implementation is:

  public CoordinateSequence create(int size, int dimension) {
    if (dimension > 3)
      dimension = 3;
      //throw new IllegalArgumentException("dimension must be <= 3");
    // handle bogus dimension
    if (dimension < 2)
        // TODO: change to dimension = 2  ???
      return new CoordinateArraySequence(size);
    return new CoordinateArraySequence(size, dimension);
  }

You can see the return new CoordinateArraySequence(size); fails to pass in the dimension.

This is a regression, in JTS 1.13 the code was as follows:

  public CoordinateSequence create(int size, int dimension) {
    if (dimension > 3)
      throw new IllegalArgumentException("dimension must be <= 3");
    return new CoordinateArraySequence(size, dimension);
  }
jodygarnett commented 7 years ago

To clarify the geotools test is testCoordinateDimensionPointLite1D and is explicitly checking if a coordinate sequence of 1 dimensional will work. This has been used, along with a 1 dimensional spatial reference system, to locate a sample along a bore hole (because geology.)

dr-jts commented 7 years ago

Er.. well, 1D geometry has never been a design goal for JTS. So I think this is more a case of GeoTools exploiting a fortuitous implementation flaw in JTS rather than a bug in supported behaviour! All kinds of things in JTS will break on 1D CoordinateSequences, so I don't see this changing anytime soon.

jodygarnett commented 7 years ago

Still messing us up, suggestions?

I can of course warp that specific factory but would prefer to have an upstream fix in JTS.

jodygarnett commented 7 years ago

If it method is not going to respect params an exception would be better than an inconsistent result.

dr-jts commented 7 years ago

Yes, agreed - this method should throw an exception for dimension < 2. Won't help the Geotools issue though.

dr-jts commented 7 years ago

I can't offer a suggestion for dealing with this, since handling 1D geometry is so far from the design goals of JTS. It really depends on what GeoTools intends for the semantics of 1D geometry. Is it possible to produce an "effective" 1D geometry by representing it as a 2D geometry with the Y coordinate set to some constant, innocuous value (e.g. 0) ?

jodygarnett commented 7 years ago

Yes, I think in that case it does the same check your getDimension() does, checks if the y value is NaN.

jnh5y commented 6 years ago

Since this is marked 'WONT FIX', can we close it?