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

ST_Aggr_ConvexHull: fix convex hull of single geometry #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wgtmac
Copy link

@wgtmac wgtmac commented Nov 3, 2021

The convex hull of a single geometry should be itself. However, the result without this patch returns an empty multi polygon.

The convex hull of a single geometry should be itself. However, the
result without this patch returns an empty multi polygon.
@randallwhitman
Copy link
Contributor

Hi @wgtmac - thanks for contributing to Spatial Framework for Hadoop. However -

The convex hull of a single geometry should be itself.

No, that's not correct in general. The straightforward counterexample is a concave polygon; its convex hull is a convex polygon, not itself.

@wgtmac
Copy link
Author

wgtmac commented Nov 4, 2021

Hi @wgtmac - thanks for contributing to Spatial Framework for Hadoop. However -

The convex hull of a single geometry should be itself.

No, that's not correct in general. The straightforward counterexample is a concave polygon; its convex hull is a convex polygon, not itself.

Thanks @randallwhitman for the explanation. I am not a geospatial expert. The problem on our side arises when we tried to aggregate multiple points to get the convex hull based on the Hive UDAF. If the partial aggregation has one single point as the input, the point is lost due to the empty multi-polygon output. Could you help fix this issue? Thanks!

@climbage
Copy link
Member

climbage commented Nov 4, 2021

This makes sense. The aggregator is assuming a convex hull can be created from a partial result.

Thinking out loud about cases where that may not hold true:

  • single point (this issue)
  • stacked [coincident] points
  • colinear points
  • straight lines

@randallwhitman randallwhitman self-assigned this Nov 4, 2021
@randallwhitman
Copy link
Contributor

randallwhitman commented Nov 4, 2021

It will be useful if we can put together a unit test for at least the case of single geometry. We used to have automatic tests of most if not all the UDF/UDAF, using a framework that is no longer available. Have started [re-]writing unit tests, but still incomplete. I might later update this comment with some links.

@randallwhitman
Copy link
Contributor

randallwhitman commented Dec 21, 2021

Please comment on this draft of a test: @wgtmac @climbage @JordanMLee

    public void TestStAggrConvexHullPoint() throws HiveException {
        ST_Point stPt = new ST_Point();
        BytesWritable bwGeom = stPt.evaluate(new DoubleWritable(1.2), new DoubleWritable(3.4));
        ST_Aggr_ConvexHull.AggrConvexHullBinaryEvaluator convexHullEval = new ST_Aggr_ConvexHull.AggrConvexHullBinaryEvaluator();
        ST_Aggr_ConvexHull.AggrConvexHullBinaryEvaluator other1 = new ST_Aggr_ConvexHull.AggrConvexHullBinaryEvaluator();
        ST_Aggr_ConvexHull.AggrConvexHullBinaryEvaluator other2 = new ST_Aggr_ConvexHull.AggrConvexHullBinaryEvaluator();
        convexHullEval.init();
        convexHullEval.iterate(bwGeom);
        BytesWritable partial = convexHullEval.terminatePartial();
        other1.merge(partial);
        convexHullEval.merge(other2.terminatePartial());
        BytesWritable result1 = other1.terminate();
        BytesWritable result2 = convexHullEval.terminate();
        OGCGeometry geom1 = GeometryUtils.geometryFromEsriShape(result1);
        OGCGeometry geom2 = GeometryUtils.geometryFromEsriShape(result2);
        assertFalse(geom1.isEmpty());
        assertFalse(geom2.isEmpty());
    }

@JordanMLee
Copy link

@randallwhitman the test above looks alright to me. When computing the aggregate convex hull, we shouldn't be losing geometries when computing the partial result or when merging. Both geom1 and geom2 should be degenerate polygons, since ST_Aggr_ConvexHull will always produce a polygon.

@randallwhitman
Copy link
Contributor

When computing the aggregate convex hull, we shouldn't be losing geometries when computing the partial result or when merging. Both geom1 and geom2 should be [non-empty] degenerate polygons, since ST_Aggr_ConvexHull will always produce a polygon.

Thanks. In fact that test is failing for me; the result is empty. Which leaves us the question, which part of the code results empty rather than non-empty-degenerate.

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

4 participants