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

Explicit CoordinateSequence support for XYZM #293

Merged
merged 11 commits into from Aug 7, 2018

Conversation

jodygarnett
Copy link
Contributor

@jodygarnett 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:

@jodygarnett
Copy link
Contributor Author

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

@jodygarnett
Copy link
Contributor Author

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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is the default interface

@jodygarnett
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@bjornharrtell
Copy link
Contributor

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

@basisbit
Copy link

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
Copy link
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
Copy link
Contributor Author

@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
Copy link
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
Copy link
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
Copy link
Contributor Author

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

@dr-jts
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
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 July 30, 2018 06:05
@jodygarnett
Copy link
Contributor Author

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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be getDimension() - getMeasures()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be + 3?

@jodygarnett
Copy link
Contributor Author

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

This example may need to be rewritten to support arbitrary dimensions and measures
*
* @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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numberOfMeasures instead of numberOfMeasures1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining typo.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would also be a valid way to represent it.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do x and y default to 0.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default constructor is:

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should standardize on the copyrights going forward.

}
}

public String toString() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I like it.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
return Double.NaN;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Picky) space after the comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be testCoordinateXYZM

Coordinate coord;
Coordinate[] array;

initProgression(seq);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the default dimension of the PackedCoordinateSequenceFactory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this affect anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jnh5y
Copy link
Contributor

jnh5y commented Aug 6, 2018

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

@jodygarnett
Copy link
Contributor Author

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
Copy link

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

@jodygarnett
Copy link
Contributor Author

jodygarnett commented Aug 7, 2018

typos, fixed comments will need to check codebase

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright clean-up.

Copy link
Contributor

@jnh5y jnh5y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jnh5y
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants