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

Stolstov/collection methods #178

Merged
merged 19 commits into from
Jun 21, 2018
Merged

Stolstov/collection methods #178

merged 19 commits into from
Jun 21, 2018

Conversation

stolstov
Copy link
Member

@stolstov stolstov commented May 28, 2018

Fixes for:
#176 #177
#179

@mbasmanova

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@stolstov Sergey, thanks for adding support for geometry collections. I'm still going through the changes, but it would be helpful to update commit messages to describe what functionality is added. I assume this PR is adding support for geometry collections input in relationship tests and distance methods.

I noticed that there are 3 commits, but first commit relies on the new file added in the third commit. Would you merge 1st and 3rd commits?

import java.util.List;

import com.esri.core.geometry.ogc.OGCConcreteGeometryCollection;
Copy link
Contributor

Choose a reason for hiding this comment

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

these imports seem unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed


@Override
public int getGeometryID() {
return m_shape.getGeometryUserIndex(m_geom, m_index);
Copy link
Contributor

Choose a reason for hiding this comment

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

m_shape can be null, in which case this will throw NPE

Copy link
Member Author

Choose a reason for hiding this comment

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

That's ok to crash as this is internal method and calling getGeometryID after iterator was exhausted is a bug.

@@ -501,4 +519,303 @@ public int hashCode() {

return hash;
}

@Override
public double distance(OGCGeometry another) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this method returns 0 if another is an empty geometry. Is this correct? Should it throw or return -1 to distinguish between the case when another and this overlap vs. another is empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Distance returns NaN for empty input. I'll fix this for collection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed


@Override
public double distance(OGCGeometry another) {
double minD = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Some other methods have if (this == another) optimization. Should it be added here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

return false;
}

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a collection of two OGCMultiPoints will incorrectly return true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

continue;
}

throw new GeometryException("internal error");//what else?
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps, clarify that unexpected type was encountered

throw new GeometryException("Internal error: unexpected geometry type: " + t);/

}

OGCMultiPoint multiPoint = null;
ArrayList<Geometry> polygons = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be just List<Geometry>


/**
* Fixes topological overlaps in the GeometryCollecion.
* This is equivalent to union of the geometry collection elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

flatten method already performs union on polygons; should flatten also union points and line strings?

}

private OGCConcreteGeometryCollection removeOverlapsHelper_(List<Geometry> geoms) {
ArrayList<Geometry> result = new ArrayList<Geometry>();
Copy link
Contributor

Choose a reason for hiding this comment

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

type of result could be just List<Geometry>

for (int j = i + 1; j < geoms.size(); ++j) {
Geometry subG = geoms.get(j);
current = OperatorDifference.local().execute(current, subG, esriSR, null);
if (current.isEmpty())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why breaking on the first geometry that doesn't overlap current? Shouldn't we check for all geometries first?

Copy link
Member Author

Choose a reason for hiding this comment

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

We break because the current - subG resulted in empty geometry, so current is completely inside of subG.


};

public static GeometryCursor prepare_for_ops_(GeometryCursor geoms, SpatialReference sr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this method simplifies the shapes. Would it be clearer to rename it to simplify or implement a SimplifyingGeometryCursor instead to match FlatteningGeometryCursor pattern?

com.esri.core.geometry.Geometry geom2 = another.getEsriGeometry();
return com.esri.core.geometry.GeometryEngine.within(geom1, geom2,
getEsriSpatialReference());
return another.contains(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

within is not the same as contains, is it? Also, why is it any different than other relationship methods? E.g. Shouldn't com.esri.core.geometry.ogc.OGCConcreteGeometryCollection ovewrite it?

Copy link
Member Author

Choose a reason for hiding this comment

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

A.contains(B) == B.within(A), so that's why the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stolstov Makes sense. With this change com.esri.core.geometry.GeometryEngine#within is no longer used. Should it be removed along with OperatorWithin and related classes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova Within/Contains, Disjoint/Intersects are a part of the API although they are trivial calls of one another. They are in OGCGeometry because they are required by the OGC spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova The primary API for this library is the one in the core.geometry package. The core.geometry.ogc is more like the facade for the use cases that require OGC spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stolstov Sergey, thanks for clarifying.

@@ -328,6 +325,10 @@ public boolean relate(OGCGeometry another, String matrix) {

// analysis
public double distance(OGCGeometry another) {
if (another.geometryType() == OGCConcreteGeometryCollection.TYPE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the same logic apply to other methods, e.g. contains, overlaps, etc.?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes most need to be changed eventually.

@@ -120,7 +121,7 @@ public OGCPoint endPoint() {

@Override
public String geometryType() {
return "LineString";
return TYPE;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is unrelated to the change that adds support for geometry collections. It would be helpful to move it into a separate commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I call geometryType() in the implementation of isFlattened. I've changed the methods to avoid string compare.

@stolstov
Copy link
Member Author

I noticed that there are 3 commits, but first commit relies on the new file added in the third commit. Would you merge 1st and 3rd commits?

I'll "squash and merge" this PR.

}

@Test
public void testIntersectsOnGeometryCollection() {
Copy link
Contributor

@mbasmanova mbasmanova May 29, 2018

Choose a reason for hiding this comment

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

I modified the test to ensure that if intersects is symmetrical and the test failed with NPE:

	@Test
	public void testIntersectsOnGeometryCollection() {
		assertIntersects("GEOMETRYCOLLECTION (POINT (1 1))", "POINT (1 1)", true);
	}

	private void assertIntersects(String wkt, String otherWkt, boolean intersects)
	{
		assertEquals(OGCGeometry.fromText(wkt).intersects(OGCGeometry.fromText(otherWkt)), intersects);
		assertEquals(OGCGeometry.fromText(otherWkt).intersects(OGCGeometry.fromText(wkt)), intersects);
	}

Here is the failure:

java.lang.NullPointerException
	at com.esri.core.geometry.RelationalOperations.relate(RelationalOperations.java:45)
	at com.esri.core.geometry.OperatorDisjointLocal.execute(OperatorDisjointLocal.java:31)
	at com.esri.core.geometry.GeometryEngine.disjoint(GeometryEngine.java:376)
	at com.esri.core.geometry.ogc.OGCGeometry.disjoint(OGCGeometry.java:275)
	at com.esri.core.geometry.ogc.OGCGeometry.intersects(OGCGeometry.java:280)
	at com.esri.core.geometry.TestOGC.assertIntersects(TestOGC.java:1007)
	at com.esri.core.geometry.TestOGC.testIntersectsOnGeometryCollection(TestOGC.java:1001)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. I'll fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@stolstov
Copy link
Member Author

@mbasmanova This PR fixes the unit tests that you listed in the issues.

//Collectively processes group of geometry collections (intersects all segments and clusters points).
//Flattens collections, removes overlaps.
//Once done, the result collections would work well for topological and relational operations.
private List<OGCConcreteGeometryCollection> prepare_for_ops_(List<OGCConcreteGeometryCollection> geoms) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@stolstov This method doesn't seem to be used. The only callers I see are calling OGCStructureInternal.prepare_for_ops_ directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've commited a change to use it. See changes to difference and intersection

@@ -955,5 +955,86 @@ public void testEmptyBoundary() throws Exception {
}
}

@Test
public void testUnionPointWithEmptyLineString() {
assertUnion("POINT (1 2)", "LINESTRING EMPTY", "GEOMETRYCOLLECTION (POINT (1 2))");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect the union of POINT (1, 2) and LINESTRING EMPTY to be POINT (1, 2), but this test asserts the result to be GEOMETRYCOLLECTION (POINT (1 2)).

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova Yes, a simpler representation would be better.

@mbasmanova
Copy link
Contributor

@stolstov Sergey, are you planning to add more tests? This PR adds a lot of new functionality, but existing tests are spotty. I took a stub at writing tests for the union and found that union of a LINESTRING (0 0, 5 0) and a POLYGON ((0 0, 5 0, 5 5, 0 5, 0 0)) returns GEOMETRYCOLLECTION (LINESTRING (0 0, 5 0), POLYGON ((0 0, 5 0, 5 5, 0 5, 0 0))) instead of POLYGON ((0 0, 5 0, 5 5, 0 5, 0 0)).

Here is the source code for the tests: TestOGCGeometryCollection.java.txt

assertUnion("LINESTRING (0 0, 5 0)", "POLYGON ((0 0, 5 0, 5 5, 0 5, 0 0))", "POLYGON ((0 0, 5 0, 5 5, 0 5, 0 0))");

fails with

org.junit.ComparisonFailure: 
Expected :POLYGON ((0 0, 5 0, 5 5, 0 5, 0 0))
Actual   :GEOMETRYCOLLECTION (LINESTRING (0 0, 5 0), POLYGON ((0 0, 5 0, 5 5, 0 5, 0 0)))

@stolstov
Copy link
Member Author

stolstov commented Jun 1, 2018

@mbasmanova Maria, thanks for the tests. I'll add them and make sure it works.

@stolstov
Copy link
Member Author

stolstov commented Jun 2, 2018

@mbasmanova Yea, the bug with polygon vs polyline is in the difference operation. Polyline.difference(polygon) does not erase polyline segments on the polygon boundary.

@stolstov
Copy link
Member Author

stolstov commented Jun 4, 2018

@mbasmanova I've created an issue for the difference: #179. I've added the fixes and your test to the PR.

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@stolstov Sergey, I started looking into contains and disjoint implementation and found some issues. How would you feel about adding a comprehensive set of tests for these operations?

@@ -81,9 +81,9 @@ static Geometry difference(Geometry geometry_a, Geometry geometry_b,
if (!env_a_inflated.isIntersecting(env_b))
return geometry_a;

if (dimension_a == 1 && dimension_b == 2)
/*if (dimension_a == 1 && dimension_b == 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why comment out and not delete this code?


OGCConcreteGeometryCollection flattened1 = flatten();
if (flattened1.isEmpty())
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic at the beginning of this method suggests that contains returns false if any of the geometries is empty. Hence, this should return false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thank you

OGCConcreteGeometryCollection otherCol = new OGCConcreteGeometryCollection(another, esriSR);
OGCConcreteGeometryCollection flattened2 = otherCol.flatten();
if (flattened2.isEmpty())
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic at the beginning of this method suggests that contains returns false if any of the geometries is empty. Hence, this should return false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

OGCGeometry g2 = flattened2.geometryN(i);
boolean good = false;
for (int j = 0, n1 = flattened1.numGeometries(); j < n1; ++j) {
OGCGeometry g1 = flattened1.geometryN(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be geometryN(j) not ..(i).

Also, it may happen that neither g1 fully contains a g2, but collectively a set of g1s contain all points in g2. Here is a test case:

public class TestOGCContains
{
    @Test
    public void testGeometryCollection()
    {
        assertContains("GEOMETRYCOLLECTION (POINT (0 0), LINESTRING (0 1, 5 1))", "GEOMETRYCOLLECTION (MULTIPOINT (0 0, 2 1))");
    }

    private void assertContains(String wkt, String otherWkt)
    {
        OGCGeometry geometry = OGCGeometry.fromText(wkt);
        OGCGeometry otherGeometry = OGCGeometry.fromText(otherWkt);
        assertTrue(geometry.contains(otherGeometry));
        assertTrue(otherGeometry.within(geometry));
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova Maria, I've added this test and fixed this. Collection contains now uses another.difference(this).isEmpty().

for (int i = 0, n1 = flattened1.numGeometries(); i < n1; ++i) {
OGCGeometry g1 = flattened1.geometryN(i);
for (int j = 0, n2 = flattened2.numGeometries(); j < n2; ++j) {
OGCGeometry g2 = flattened2.geometryN(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be geometryN(j) not ..(i).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

@stolstov I'm still seeing OGCGeometry g2 = flattened2.geometryN(i); using i and not j.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova Sorry about that. I've missed this place. Please, change it locally if you plan testing this, I'll fix this later tonight.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova Should be fixed now. I've added a test for this case.

}

public boolean contains(OGCGeometry another) {
if (another.geometryType() == OGCConcreteGeometryCollection.TYPE) {
return another.contains(this);
Copy link
Contributor

@mbasmanova mbasmanova Jun 7, 2018

Choose a reason for hiding this comment

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

this.contains(another) is not equal to another.contains(this), is it?

The following test in TestOGCContains.java.txt fails:

assertContains("MULTIPOINT (1 2, 3 4)", "GEOMETRYCOLLECTION (POINT (1 2))");

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova Maria, I've forked the repo to continue cleanup with the fixes for the OGC collection here: https://github.com/stolstov/geometry-api-java/tree/ogc_fixes. I've added you as a contributor on the fork as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stolstov Sergey, thanks. How would you like us to use the fork?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mbasmanova I'd like to use the fork to finish this PR. Once done, I'll merge the forked branch here and then merge this PR. It would also make it simpler for you if you wish to add a commit.

@mbasmanova
Copy link
Contributor

@stolstov

A couple of issues with the intersection method:

  • Intersection of GEOMETRYCOLLECTION (POINT (1 2)) and POINT EMPTY throws "GeometryException: internal error".

  • Intersection of GEOMETRYCOLLECTION (POINT (1 2), MULTIPOINT (3 4, 5 6)) and POINT (3 4) returns GEOMETRYCOLLECTION (POINT (3 4)) and not POINT (3 4)

@mbasmanova
Copy link
Contributor

@stolstov Sergey, I was busy with prestodb/presto#10731 and somewhat lost track of this PR. What are the next steps?

@stolstov
Copy link
Member Author

@mbasmanova I'll merge once you and @randallwhitman are OK it.

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@stolstov Sergey, thank you so much for expanding contains, within, intersects, disjoint, distance, difference and intersections operations to support geometry collections. Can't wait to make these available to Presto users.

@@ -955,5 +954,86 @@ public void testEmptyBoundary() throws Exception {
}
}

@Test
public void testUnionPointWithEmptyLineString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove this test; it is already included in TestOGCGeometryCollection#testUnionPoint

}

@Test
public void testUnionPointWithLinestring() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove this test; it is already included in TestOGCGeometryCollection#testUnionPoint

}

@Test
public void testUnionLinestringWithEmptyPolygon() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this to TestOGCGeometryCollection#testUnionLineString

}

@Test
public void testUnionLinestringWithPolygon() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this to TestOGCGeometryCollection#testUnionLineString

}

@Test
public void testUnionGeometryCollectionWithGeometryCollection() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this to TestOGCGeometryCollection

}

@Test
public void testContainsOnGeometryCollection() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this to TestOGCContains

}

@Test
public void testDistanceOnGeometryCollection() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this to TestOGCDistance


@Test
public void testFlattened() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this to TestOGCGeometryCollectionFlatten

import org.junit.Assert;
import org.junit.Test;

public class TestOGCGeometryCollection {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename this class to TestOGCUnion to match other test classes


public class TestOGCGeometryCollection {
@Test
public void testUnionPoint() {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's drop Union from method names and rely on the class name TestOGCUnion to provide the necessary context

Copy link
Contributor

@randallwhitman randallwhitman left a comment

Choose a reason for hiding this comment

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

I defer to Sergey and Maria.

@stolstov stolstov merged commit ecaedf4 into master Jun 21, 2018
@stolstov stolstov deleted the stolstov/collection_methods branch June 21, 2018 20:45
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

Successfully merging this pull request may close these issues.

None yet

4 participants