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

Explicit CoordinateSequence support for XYZM #293

Merged
merged 11 commits into from Aug 7, 2018

Conversation

Projects
None yet
9 participants
@jodygarnett
Contributor

jodygarnett commented Jul 1, 2018

Checklist:

  • CoordinateSequence.getMeasures() default method
  • Update to PackedCoordinateSequence
  • discussion and feedback
  • Introduce Coordinate accessors for getX(), getY(), getZ(), getM()
  • Introduce explicit CoordinateXY, CoordinateXYM, CoordinateXYZM
  • Updated CoordinateTest to cover subclasses
  • Added CoordinateSequence hasZ and hasM default methods to perform common dimension and measures checks
  • Updated PackedCoordinateSequenceTest to try out XY, XYM, XYZ, XYZM functionality
  • Updated CoordinateArraySequence as well
  • Added CoordinateSequence.createCoordinate() default method, to assist with client use
  • Reworked ExtendedCoordinateSequence example

Ideally:

  • proof of fitness with feedback from #291 WKT Reader/Writer enhancement
@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 1, 2018

This PR is for discussion and feedback and is not ready to be merged.

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 17, 2018

So reading some of the javadocs indicates that it is up to the client code to assign a meaning to dimensions beyond 2.

Son in GeoTools if we have a SRS with dimension 2 and a geometry with dimension 3 we would know we are looking at XYM data.

Based on this we could abandon this PR, but it would mean that only client application could implement WKT Reader/Writer ...

I think I would prefer to make the explicit API change.

* Returns the number of measures in each coordinate for this sequence.
* @return
*/
default int getNumberOfMeasures() {

This comment has been minimized.

@jodygarnett

jodygarnett Jul 17, 2018

Contributor

here is the default interface

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 17, 2018

Discussion in JTS meeting - make an API change:

  • CoordinateSeqeunce.getNumberOfMeasures()
  • Coordinate.getZ() - needs some logic based on number of measures and dimension
  • Coordinate.getM() - needs some logic based on number of measures and dimension
@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 17, 2018

@bjornharrtell is the above use of default interfaces going to be okay for transpiling

@bjornharrtell

This comment has been minimized.

Contributor

bjornharrtell commented Jul 17, 2018

Not sure, should be possible to fix in case of troubles, so don't let it be a blocker.

@basisbit

This comment has been minimized.

basisbit commented Jul 17, 2018

could you please specify what Z and M should represent? I guess for Z this is obvious for many use cases, some clarification would still be nice.

@dr-jts

This comment has been minimized.

Contributor

dr-jts commented Jul 17, 2018

Z is height (to support "2.5 D" - it's not intended to support true 3D at this time).

M is a Measure. The meaning of this is user-dependent, Example uses include: a time value along a trajectory; a "driven distance" along a linear track (e.g. one that takes into account terrain distance).

The only semantics JTS will attach to measures is that it is assumed they can be linearly interpolated along line segments.

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 17, 2018

@basisbit I think we would stick with the SFSQL definition:

Reading the spec they relatively few references to "M" and do not define:

  • GEOMETRY_TYPE — the type of geometry values stored in this column. The use of a non-leaf Geometry class name from the Geometry Object Model for a geometry column implies that domain of the column corresponds to instances of the class and all of its subclasses. The suffixes "Z", "M" and "ZM" are three distinct copies of the geometry hierarchy as presented in Figure 4. If the value is NULL, then the appropriate GEOMETRY subtype is used consistent with the COORD_DIMENSION and SRID is implied.
  • COORD_DIMENSION — the number of ordinates used in the complex, usually corresponds to the number of dimensions in the spatial reference system. If an "M" ordinate is included it shall be one greater than the number of dimensions of the spatial reference system.
@FObermaier

This comment has been minimized.

Contributor

FObermaier commented Jul 18, 2018

Some questions:

  • CoordinateSequence.getNumberOfMeasures
    Does the introduction of getNumberOfMeasures imply changing the meaning of getDimensions (as in GeoTools' CoordinateAccess interface)? If so this would impose a breaking change for CoordinateSequence implementations like PackedCoordinateSequence that are used with a dimension greater than 3.
  • Coordinate.getZ()
    Why, Coordinate already has a z field that we can publicly access?
  • Coordinate.getM()
    Adding a measure member to the Coordinate class bloats it additionally. Coordinate.z is not used for the most part. Or do you want to add some reference to the sequence it belongs to? Either way, I don't think I like it. Access of measure values through the CoordinateSequence is sufficient IMHO.
@dr-jts

This comment has been minimized.

Contributor

dr-jts commented Jul 18, 2018

@FObermaier my thoughts on answers to your questions:

  • the semantics of CoordinateSequence.getDimension should not change. It should continue to return the total number of ordinates available.
  • I think @jodygarnett meant CoordinateSequence.getZ. This returns NaN if no Z dimension was defined.
  • Again, I think the suggestion is to add CoordinateSequence.getM.

The Z and M accessors are suggested in order to make the support for Z and M explicit in the API (and to match the explicit support in the SFSQL spec).

It might be good to provide a CoordinateSequence.hasZ method, in order to encapsulate the logic of determining this (and this can be made a default interface method). hasM could be provided as well, but does have the fairly trivial equivalent of getNumberOfMeasures == 1

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 18, 2018

Guess I am always confused if M counts as a ordinate with respect to getDimension().

@dr-jts

This comment has been minimized.

Contributor

dr-jts commented Jul 18, 2018

Yeah, it's confusing. In JTS dimension has always referred to the total number of ordinates available. If we change that not sure what will happen...

If getDimension is changed to be just the number of geometric dimensions, then pretty sure that a getNumberOfOrdinates should be available as well, for ease of iteration.

@dr-jts

This comment has been minimized.

Contributor

dr-jts commented Jul 18, 2018

Further to the question of the semantics of getDimension, my read of the SFSQL (below) is that getDimension should be the total number of ordinates, including the M value(s) if present, based on the second sentence.

COORD_DIMENSION — the number of ordinates used in the complex, usually corresponds to the number of dimensions in the spatial reference system. If an "M" ordinate is included it shall be one greater than the number of dimensions of the spatial reference system.

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 18, 2018

You can see my quote above about GEOMETRY_TYPE having different values for POINT POINTZ POINTM POINTZM. If we went that approach and reported back the correct SFSQL type .. it would be a pain and not be visible at the CoordinateSequence level.

I would like to keep getDimension() in agreement with the SFSQL spec if we can:

dimension = spatialDimension + numberOfOrdinates

Would rather see an approach using numbers, rather than hasM() and hasZ() methods .. to allow for XYZMN etc...

@dr-jts

This comment has been minimized.

Contributor

dr-jts commented Jul 18, 2018

I don't understand dimension = spatialDimension + numberOfOrdinates. What is numberOfOrdinates ?

Agreed that a numeric approach is needed (as you say, to allow for measures > 1). But doesn't preclude having the predicates too.

@jodygarnett jodygarnett requested review from jnh5y and dr-jts Jul 30, 2018

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Jul 30, 2018

@dr-jts and @jnh5y can I ask for a review, I have responded I think to most of the feedback.

I ended up having CoordinateSequence.getDimension() as described in the conversation above, and CoordinateSequence.getMeasures() being the number of measures (so if we have XYZM dimension is 4 and measures is 1.

I also removed the mutability from PackedCoordinateSequence as the factory methods were provided, and there was no need for PackagedCoordinateSequence to hold on to state (especially when their were static constances for PackagedCoordinateSequence.DOUBLE and PackagedCoordinateSequence.FLOAD.

@jnh5y I have introduced the specific Coordinate subclasses as you requested, so we end up with:

  • Coordinate - stores XYZ, the getM() method always returns Double.NaN
  • CoordinateXY - both getZ() and getM() return Double.NaN. The field "z" is still visible from superclass but not used
  • CoordinateXYM
  • CoordinateXYZM
public static final int M = 3;
/**
* The x-ordinate.
*/
public double x;

This comment has been minimized.

@elahrvivaz

elahrvivaz Jul 30, 2018

would it make sense to deprecate direct access to x and y as well? It seems potentially confusing to have coord.x, coord.y and coord.getZ() be the established access pattern

This comment has been minimized.

@jodygarnett

jodygarnett Jul 31, 2018

Contributor

I would like to do so, but discussion with @jnh5y and @dr-jts has me considering if having explicit classes, with access to the primitives, is required. Martin quite correctly focuses on these being primary a data structures, rather than an object.

With java value types on the horizon we are likely to change to the struct idea from C. Really want to have coordinates represent a block of [xy], [xyz], [xym], [xyzm] if continuous memory, so when we make an array of them everything is co-located in memory.

This comment has been minimized.

@jodygarnett

jodygarnett Jul 31, 2018

Contributor

So my design hat agrees with you, prefer getX() and getY(), make them final so the JRE can inline them to avoid performance penalty which is a key concern ...

This comment has been minimized.

@dr-jts

dr-jts Jul 31, 2018

Contributor

Have to keep public access to Coordinate x and y fields for backwards compatibility. These could be removed in a future non-compatible version (i,e, JTS 2)

This comment has been minimized.

@dr-jts

dr-jts Jul 31, 2018

Contributor

Well, unless a big refactoring is done (as I see @jodygarnett has already done for the z field).

This comment has been minimized.

@jodygarnett

jodygarnett Aug 1, 2018

Contributor

really? I thought it just made compiler warnings?

From the annotation javadocs:

A program element annotated @deprecated is one that programmers
are discouraged from using, typically because it is dangerous,
or because a better alternative exists. Compilers warn when a
deprecated program element is used or overridden in non-deprecated code.

In anycase I have it commended it as deprecated as the javadoc approach allows a message to be provided

This comment has been minimized.

@dr-jts

dr-jts Aug 1, 2018

Contributor

Well, yes, warnings not errors. Still, it's nice to allow compiling warning-free.

This comment has been minimized.

@elahrvivaz

elahrvivaz Aug 1, 2018

my $0.02 - probably a good idea to comment and also mark them. the compiler warnings are useful to raise awareness that users should be moving away from the methods, whereas most wouldn't notice a comment change and then face an unexpected compile failure whenever they actually go away.

This comment has been minimized.

@dsmiley

dsmiley Aug 1, 2018

LOL emitting warnings so people change their code is entirely the point. Then they change their code and the warnings go away

This comment has been minimized.

@dr-jts

dr-jts Aug 1, 2018

Contributor

I have been in situations where compile-time warnings trigger a rejection of the update to the library, and it is not possible to change the client code. So I'm very wary of deprecating things until at least a few releases containing the alternative functionality.

*/
default double getM(int index) {
if( getDimension() > 2 && getMeasures() > 0 ) {
final int mIndex = getDimension()-getDimension();

This comment has been minimized.

@elahrvivaz

elahrvivaz Jul 30, 2018

should this be getDimension() - getMeasures()?

This comment has been minimized.

@jodygarnett

jodygarnett Jul 31, 2018

Contributor

yes, yes it should. And I should have test cases before this goes live :)

return 2;
}
else if(coordiante instanceof CoordinateXYM) {
return 2;

This comment has been minimized.

@elahrvivaz

elahrvivaz Jul 30, 2018

should this be 3?

coords[i * this.dimension + 2] = coordinates[i].z;
coords[i * this.dimension + 2] = coordinates[i].getOrdinate(2); // Z or M
if (this.dimension >= 4)
coords[i * this.dimension + 2] = coordinates[i].getOrdinate(3); // M

This comment has been minimized.

@elahrvivaz

elahrvivaz Jul 30, 2018

should this be + 3?

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Aug 1, 2018

Okay folks test case written, this has collected all feedback provided.

* @return true if {@link #getM(int)} is supported.
*/
default boolean hasM() {
return getDimension()>2 && getMeasures() > 0;

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

@jodygarnett is there any used case for 1-d geometries with a measure? Is checking getMeasures() > 0 sufficient?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

There is in GeoTools (boreholes return a 1 dimensional offset along a 3D line string).
The result is mostly used as a data structure, although I suppose you could do some operations.

This functionality was "broken" in JTS 1.14, GeoTools now makes use of its own factory to create points with only one ordinate.

*
* @param index the coordinate index in the sequence
* @param ordinateIndex the ordinate index in the coordinate (in range [0, dimension-1])
* @param ordinateIndex the ordinate index in the coordinate (in range [0, dimension-numberOfMeasures-1])

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

I'm not sure about the range here. It seems to me that the range would still be [0, dimension-1].

The bit from [0, dimension-numberOfMeasures-1] would be the XY(Z) values. For numberOfMeasures > 0, the bit from [dimension-numberOfMeasures, dimension-1] would the measures.

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

Yeah I think you are right, since we did not end up with a separate getMeasure method.

@param ordinateIndex the ordinate index in the coordinate (in range [0, dimension-1])

This comment has been minimized.

@jnh5y

jnh5y Aug 7, 2018

Contributor

Should you update this line of documentation then?

@@ -133,7 +221,7 @@
* Sets the value for a given ordinate of a coordinate in this sequence.
*
* @param index the coordinate index in the sequence
* @param ordinateIndex the ordinate index in the coordinate (in range [0, dimension-1])
* @param ordinateIndex the ordinate index in the coordinate (in range [0, dimension-numberOfMeasures1])

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

numberOfMeasures instead of numberOfMeasures1

This comment has been minimized.

@jnh5y

jnh5y Aug 7, 2018

Contributor

Remaining typo.

case X: return x;
case Y: return y;
}
throw new IllegalArgumentException("Invalid ordinate index: " + ordinateIndex);

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Should this be the 'default' case in the switch?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

that would also be a valid way to represent it.

/** Default constructor */
public CoordinateXYM() {
super();
this.m = 0.0;

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Do x and y default to 0.0?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

Default constructor is:

Constructs a Coordinate at (0,0,NaN).

public CoordinateXYM(Coordinate coord) {
super(coord.x,coord.y);
m = getM();

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Can you remind what the default value for an uninitialized double is?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

inconsistent, 0 for a spatial dimension (XY), DnD for an optional value (Z)

@@ -0,0 +1,96 @@
/*
* Copyright (c) 2016 Vivid Solutions.

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

We should standardize on the copyrights going forward.

}
}
public String toString() {

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

On the topic of the 'toString' methods... It seems that the original Coordinate.toString has ()'s. Should we be careful here?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

So original is Coordinate:

    return "(" + x + ", " + y + ", " + z + ")";

And ExtendedCoordinate:

    String stringRep = x + " " + y + " m=" + m;
    return stringRep;

I am happy to go with '(', kind of like the use of 'm=' so it is clear when measures are in play.

This comment has been minimized.

@jnh5y

jnh5y Aug 7, 2018

Contributor

+1, I like it.

} else if (dimension == 4 && measures == 1) {
return new CoordinateXYZM();
}
return new Coordinate();

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Hmm... This seems a tough case to handle properly...

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

I think I should make a CoordinateSequence.createCoordinate() method, it will make the ExtendedCoordinateSequence example easier to work with as well. If not we are demanding that these subclasses be used which is poor design.

@@ -153,7 +201,12 @@ public Coordinate getCoordinateCopy(int i) {
public void getCoordinate(int index, Coordinate coord) {

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

As a sanity check, this seems like an easy place for a user to do something silly. If they pass in the wrong kind of Coordinate, they could get a no-Z/M-support error message, right?

In JTS 2.0, it'd be great to see if we can be clever enough to handle this at the type-system.

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

In this case we trust the coord set methods to handle things appropriately.

} else {
return Double.NaN;
}

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Extra newline.

@@ -178,9 +256,9 @@ public double getOrdinate(int index, int ordinateIndex)
switch (ordinateIndex) {
case CoordinateSequence.X: return coordinates[index].x;
case CoordinateSequence.Y: return coordinates[index].y;
case CoordinateSequence.Z: return coordinates[index].z;
default:
return coordinates[index].getOrdinate(ordinateIndex);

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Sanity check, does one get Double.NaN or an exception if they go out of bounds?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

found it was a little inconsistent. My understanding is exception if you are out of bounds, DnD if you are in bounds buy the value is undefined.

if (dimension < 2)
dimension = 2; // handle bogus dimension
if (dimension - measures < 2) {
throw new IllegalArgumentException("max spatial dimension 2 required");

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Do you mean 'Minimum spatial dimension of 2 required'? (I'm also a fan of dumping the inputs. E.g. "Minimum spatial dimension of 2 required. Input dimension was $dimension, input number of measures was $measures.")

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

fixed

@@ -271,14 +300,14 @@ public Double(Coordinate[] coordinates, int dimension) {
* @param coordinates
*/
public Double(Coordinate[] coordinates) {
this(coordinates, 3);
this(coordinates, 3,0);

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

(Picky) space after the comma.

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

aside: I could not find any code formatting guidelines for the project, I setup some defaults that kind of matched in my IDE an hoped for the best.

assertEquals( xym, coord );
assertTrue( !xym.equalInZ(coord,0.000001) );
}
public void testCoordinateXYMZ() {

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Should be testCoordinateXYZM

Coordinate coord;
Coordinate[] array;
initProgression(seq);

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Is there an easy way to share this test code with the other class?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

Could place it in the superclass, the two tests are slightly different though. Since one store the coordinate instances (so == returns true) and one does not (so == returns false).

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Could have an override-able method around that check?

@@ -151,7 +151,7 @@ private void runWKBTest(String wkt) throws IOException, ParseException
private void runWKBTestPackedCoordinate(String wkt) throws IOException, ParseException {
GeometryFactory geomFactory = new GeometryFactory(
new PackedCoordinateSequenceFactory(PackedCoordinateSequenceFactory.DOUBLE, 2));
new PackedCoordinateSequenceFactory(PackedCoordinateSequenceFactory.DOUBLE));

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

What's the default dimension of the PackedCoordinateSequenceFactory?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

-dimension 3, measures 0, you can see in the constructors.-

update: the above was incorrect, there is no default code is intended to use the factory to create the coordinate sequence they want.

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Then this is a change, no? Here we are testing getting a PCSF which creates things of dimension 2, right?

Also, a WKBReader should be able to return values which have any 2, 3, and 4 dimensions...

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

Oh here is the deal, the previous implementation of the factory was modal! So it held on to the init value 2 and would use it as a default for some of the create methods.

I removed this modal functionality since it introduced the possibility for error, especially with the static final factory implementations. Any thread could change the "default" messing up another thread.

So the above change is on purpose.

This comment has been minimized.

@jnh5y

jnh5y Aug 7, 2018

Contributor

+1

pom.xml Outdated
@@ -193,7 +193,7 @@
<version>2.15</version>
<configuration>
<includes>
<include>**/*Test.java</include>
<include>**/*Test.`java`</include>

This comment has been minimized.

@jnh5y

jnh5y Aug 6, 2018

Contributor

Does this affect anything?

This comment has been minimized.

@jodygarnett

jodygarnett Aug 6, 2018

Contributor

eek! probably. Just reverted it, everything still builds.

@jnh5y

This comment has been minimized.

Contributor

jnh5y commented Aug 6, 2018

@jodygarnett ok, I finished a read-through. Looks pretty close.

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Aug 6, 2018

Thanks @jnh5y I will update based on your feedback, and then we really have to call it done as I do not have more time to put towards it.

Based on the above how do you feel about:

  • CoordinateSequence.createCoordinate()
  • Either going ahead with (c) Vivid Solutions, and others or reverting to (c) Vivid Solutions. Can you ask and let us know what to do?
@jratike80

This comment has been minimized.

jratike80 commented Aug 7, 2018

I can read "coordiante" in quite many places, like CoordianteSequence.createCoordiante(). Are they typos or something intentional?

@jodygarnett

This comment has been minimized.

Contributor

jodygarnett commented Aug 7, 2018

typos, fixed comments will need to check codebase

@@ -0,0 +1,90 @@
/*
* Copyright (c) 2016 Vivid Solutions

This comment has been minimized.

@jnh5y

jnh5y Aug 7, 2018

Contributor

Copyright clean-up.

@jnh5y

jnh5y approved these changes Aug 7, 2018

There are some minor typos left to address. Also, we should update the copyright headers before merging.

@jnh5y

This comment has been minimized.

Contributor

jnh5y commented Aug 7, 2018

@dr-jts Any more comments on this? I'd like to merge this PR later this week if there are no understanding issues.

@jodygarnett jodygarnett merged commit 412bf17 into locationtech:master Aug 7, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment