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

add DE-9IM predicates and test #13

Merged
merged 10 commits into from
Oct 15, 2023
Merged

add DE-9IM predicates and test #13

merged 10 commits into from
Oct 15, 2023

Conversation

rafaqz
Copy link
Owner

@rafaqz rafaqz commented Aug 1, 2023

@evetion these might help with JuliaGeo/GeoInterface.jl#106

There are a few more to add, we have intersects, within, contains, union and intersection so far.

In GeoInterface we can just do things like

within(geom, ext::Extent) = Extents.within(GI.extent(geom), ext)

@rafaqz rafaqz changed the title add contains and within methods and test add contains and within methods and test Aug 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #13 (a743a1b) into main (b76338a) will increase coverage by 3.36%.
The diff coverage is 87.87%.

@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
+ Coverage   84.40%   87.76%   +3.36%     
==========================================
  Files           1        1              
  Lines         109      139      +30     
==========================================
+ Hits           92      122      +30     
  Misses         17       17              
Files Coverage Δ
src/Extents.jl 87.76% <87.87%> (+3.36%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@evetion evetion left a comment

Choose a reason for hiding this comment

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

Sorry to take this long in reviewing this, these things are hard to quickly review (on mobile), it requires a bit of careful thought.

Nice to expand the spatial predicates. You could add disjoint quite easily, as it's the inverse of intersects.

Any plans on adding equals (should be Base.isequal, except for strict ofcourse), and or others (touches is mina==maxb or maxa==minb), and there is overlaps (intersects with > instead of >=, but not equal)?

Some comments.

src/Extents.jl Outdated
end
end

_bounds_intersect(b1::Tuple, b2::Tuple) = (b1[1] <= b2[2] && b1[2] >= b2[1])# || (b2[1] <= b1[2] && b2[2] >= b1[1])
Copy link
Collaborator

@evetion evetion Aug 13, 2023

Choose a reason for hiding this comment

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

Still commented out code here to remove that should be included? Doesn't seem to be complete yet.

Might be good to have a comment like
# abab or baba (which also matches abba and baab, but not aabb or bbaa)
and/or even destructure the tuples to (mina, maxa), (minb, maxb) for readability.

Note that this is sometimes written as the inverse (maxa < minb) || (mina > maxb)

test/runtests.jl Outdated
d = Extent(A=(0.0, 1.0))
d = Extent(X=(0.2, 0.45))
e = Extent(A=(0.0, 1.0))

Copy link
Collaborator

Choose a reason for hiding this comment

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

I love the extensive tests ❤️ . However, we might need to add even more, seeing the missed cases for intersects, and also to test the equal in the <=?

It might be useful to comment specific cases where you test something very specific (like an equality edgecase)?

@rafaqz
Copy link
Owner Author

rafaqz commented Aug 13, 2023

Yes to disjoint and overlaps, and I guess equals.

Also to very clear tests and docs. This package should be bomb-proof.

But not sure when I will be able to lift it to that standard.

@rafaqz
Copy link
Owner Author

rafaqz commented Sep 28, 2023

@evetion I went all out ISO and implemented DE-9IM

Seems to work well but some feedback would be good. I'm not sure about the strict keyword and generally what to do as the default when there is an extra dimension in one of the extents (like there is a time or Z dimension in a spatial raster as well as X/Y - what do when checking if it intersects with a regular X/Y raster.

They all seem to take a few nanosecond. I'm thinking we can use them in fast loops over many geometries, matching some predicate like everything that might overlap.

@rafaqz rafaqz changed the title add contains and within methods and test add DE-9IM predicates and test Sep 28, 2023
Copy link
Collaborator

@evetion evetion left a comment

Choose a reason for hiding this comment

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

Nice work! It's a pain to get all these right. 👍

src/Extents.jl Outdated Show resolved Hide resolved
src/Extents.jl Outdated Show resolved Hide resolved
src/Extents.jl Outdated Show resolved Hide resolved
src/Extents.jl Show resolved Hide resolved
@rafaqz rafaqz merged commit e75cbf7 into main Oct 15, 2023
9 checks passed
@rafaqz rafaqz deleted the within_contains branch October 15, 2023 20:09
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.

3 participants