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

Two empty geometries should have the same topology #1087

Closed
michaelkirk opened this issue Oct 9, 2024 · 9 comments
Closed

Two empty geometries should have the same topology #1087

michaelkirk opened this issue Oct 9, 2024 · 9 comments

Comments

@michaelkirk
Copy link

For example:

MultiPoint a = (MultiPoint) readerXY.read("MULTIPOINT EMPTY");
MultiPoint b = (MultiPoint) readerXY.read("MULTIPOINT EMPTY");
// This currently fails
assertTrue(a.equalsTopo(b));

Does the current behavior make sense?

@dr-jts
Copy link
Contributor

dr-jts commented Oct 9, 2024

I think the original idea was that EMPTY geometries contained no points, so could not be equal. But it's debatable whether that makes sense.

@dr-jts
Copy link
Contributor

dr-jts commented Oct 9, 2024

This was changed in GEOS for RelateNG so that empty geometries are equal (regardless of geometry type).

@michaelkirk
Copy link
Author

michaelkirk commented Oct 9, 2024

The motivation for my question (which I should have included originally, sorry!) is that I'm comparing the outputs of some geometry transformations to known "correct" answers.

Occasionally the expected answer is an empty geometry.
So to check that I have achieved the correct answer, I currently have to say:

// (pseudo code)
if expected.empty() && actual.empty() {
   return true
} else {
   return expected.isTopoEquals(actual)
}

I'm somewhat emboldened by this change in RelateNG - it's what I expected.

@dr-jts
Copy link
Contributor

dr-jts commented Oct 9, 2024

That's a good use case for this semantic.

This actually isn't implemented in JTS yet. Perhaps I should make that change.

michaelkirk added a commit to georust/geo that referenced this issue Oct 10, 2024
Note, this is a divergence from JTS.Geometry.isEqualTopo, but it seems
like the right thing to do.

Supportive discussion with Dr. JTS:
locationtech/jts#1087 (comment)

It also aligns with the new Overlay semantics in GEOS (OverlayNG).
michaelkirk added a commit to georust/geo that referenced this issue Oct 10, 2024
Note, this is a divergence from JTS.Geometry.isEqualTopo, but it seems
like the right thing to do.

Supportive discussion with Dr. JTS:
locationtech/jts#1087 (comment)

It also aligns with the new Overlay semantics in GEOS (OverlayNG).
@michaelkirk
Copy link
Author

For my own reference, in geos, it looks like we're talking about this code:

class EqualsTopoPredicate : public IMPredicate {

    std::string name() const override {
        return std::string("equals");
    }

    bool requireInteraction() const override {
        //-- allow EMPTY = EMPTY
        return false;
    };

    void init(int _dimA, int _dimB) override {
        IMPredicate::init(_dimA, _dimB);
        //-- don't require equal dims, because EMPTY = EMPTY for all dims
    }

    void init(const Envelope& envA, const Envelope& envB) override {
        //-- handle EMPTY = EMPTY cases
        setValueIf(true, envA.isNull() && envB.isNull());

        require(envA.equals(&envB));
    }
    ...
}

https://github.com/libgeos/geos/blob/29a4c7ea895da8bb721f1d9342b0a8ef3ad89b2f/include/geos/operation/relateng/RelatePredicate.h#L505

@dr-jts
Copy link
Contributor

dr-jts commented Oct 10, 2024

For my own reference, in geos, it looks like we're talking about this code:

https://github.com/libgeos/geos/blob/29a4c7ea895da8bb721f1d9342b0a8ef3ad89b2f/include/geos/operation/relateng/RelatePredicate.h#L505

Correct.

@dr-jts
Copy link
Contributor

dr-jts commented Oct 11, 2024

I changed RelateNG to have the EMPTY-equals semantics (#1089)

I also added an XML test TestRelateEmpty.xml for various EMPTY cases (since this was lacking, surprisingly). @michaelkirk are you making use of these in georust?

@michaelkirk
Copy link
Author

Thank you for making this change.

I wrote a test harness to consume your xml tests here: https://github.com/georust/geo/tree/main/jts-test-runner

It was especially useful when I was porting the topology graph stuff for DE-9IM operations. I haven't yet found the time to port your OverlayNG work.

I'll pull in the new https://github.com/locationtech/jts/blob/master/modules/tests/src/test/resources/testxml/misc/TestRelateEmpty.xml momentarily.

@michaelkirk
Copy link
Author

The new tests uncovered a couple errors in our own implementation, so thank you for those. All our tests are passing now.

https://github.com/georust/geo/pull/1227/files

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

No branches or pull requests

2 participants