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.

Sign up for GitHub

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

Jump to bottom

Geometry corrections (unofficial dev branch) #69

Merged
merged 23 commits into from
Mar 27, 2024
Merged

Geometry corrections (unofficial dev branch) #69

merged 23 commits into from
Mar 27, 2024

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Mar 5, 2024 •
edited
Loading

These are the geometry correction and type stability changes forked out from #64.

@asinghvi17
As/correction and benchmarking (#68)
039ea5c 
* Add geometry correction framework

* Basic benchmarking

* Fix example in ClosedRing

* Fix the geometry correction interface

* Add benchmarks!

* fix doc build

* fix again

* Literatify with Documenter.jl

* Add all necessary packages to `docs/Project.toml`

* Minor fixes

* Fix typo in utils script

* OGC benchmarks

* Add a couple more benchmarks, don't actually execute on CI

* Yet more benchmarks

* Correct type instability when using booleans-as-types in `threaded`

* Add `within` benchmark

* More benchmarks!
@asinghvi17
Remove benchmarking code
e2226d6
@asinghvi17 asinghvi17 mentioned this pull request Mar 5, 2024
Merged
@skygering skygering mentioned this pull request Mar 6, 2024
Closed
skygering
skygering reviewed Mar 6, 2024
View reviewed changes
src/methods/centroid.jl Outdated Show resolved Hide resolved
skygering
skygering reviewed Mar 6, 2024
View reviewed changes
Copy link
Collaborator

@skygering skygering left a comment

Choose a reason for hiding this comment

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

Looks good overall! Excited to have this on its way. We should add some tests though.

@asinghvi17 asinghvi17 mentioned this pull request Mar 10, 2024
Merged
@asinghvi17 asinghvi17 changed the title Geometry corrections + a bit of type stability Geometry corrections Mar 10, 2024
@asinghvi17
Copy link
Member Author

The type instability fix from here also exists in #74 so I'll make this one purely about geometry corrections

@asinghvi17 @github-actions @rafaqz
Line segmentization/densification methods (#71)
3bb6fb6 
* CompatHelper: add new compat entry for Statistics at version 1, (keep existing compat) (#70)

Co-authored-by: CompatHelper Julia <compathelper_noreply@julialang.org>

* Add segmentization

* Fix method ambiguity and benchmarks

* Squash type instabilities and get the benchmark going

* Fix plot in display

* Apply suggestions from code review

Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>

* Add tests

* Add benchmark plotting functionality to docs

* In-file benchmark for simplify

* Reorganize the polygonize file to use multiline comments

* get tests running

* Fix area for linear rings

* Fix segmentize doubling points unnecessarily

* Fix some simplify test errors

* Add benchmark plots file to the top level

* Switch area for linear rings back to 0

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: CompatHelper Julia <compathelper_noreply@julialang.org>
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
@asinghvi17
Copy link
Member Author

asinghvi17 commented Mar 21, 2024 •
edited
Loading

I guess this is the dev branch now :D. It will be ideal to rebase and merge instead of squashing, since this touches a lot of code and I want to keep the history intact if possible.

@asinghvi17
Implement a Proj extension
f576a6d 
This implements a package extension for Proj and avoids us having to depend explicitly on it.  With this, GeometryOps has no explicit dependencies on C libraries!  We might also extend the `reproject` interface to allow e.g. FastGeoProjections and other projection interfaces.

The issue with this is that the GeodesicSegments struct holds a Proj.geod_geodesic object.  That field is currently untyped, we may choose to type it and only accept GeodesicSegments{Proj.geod_geodesic} in principle.
@asinghvi17
Remove ExactPredicates Dependance (#72)
5d57ddf 
When I was profiling a case of `intersection` between two polygons with holes, I noticed that a LOT of time was spent on the calls to `ExactPredicates.meets`. It is generated code and for some reason didn't seem to be doing that efficiently.  That can be seen in the left and right (type stable) towers below:
![](https://github.com/asinghvi17/GeometryOps.jl/assets/60117338/6f7ce2ca-e7d4-4752-81f7-da973df07d43)

Here was the performance with `ExactPredicates`:
![](https://github.com/asinghvi17/GeometryOps.jl/assets/60117338/74f4ed4e-7949-46c2-8c1f-c14c71f4bfac)

I realized that I had code that was serving a similar purpose with `segment_segment_orientation` and that if I developed the `_intersection_points` code to cover all cases, then we had all of the needed functionality. After removing `ExactPredicates`, here is the performance of the same polygons:
![](https://github.com/asinghvi17/GeometryOps.jl/assets/60117338/9475b9ba-1f1c-4b1a-b5fc-d70a37d92da9)

You can see my issue on the type instability in the above profile in #73.

Co-committed-by: Skylar Gering <sgering@g.hmc.edu>
@asinghvi17 asinghvi17 mentioned this pull request Mar 21, 2024
Merged
@asinghvi17 asinghvi17 changed the title Geometry corrections Geometry corrections (unofficial dev branch) Mar 21, 2024
@asinghvi17 asinghvi17 mentioned this pull request Mar 22, 2024
Closed
@asinghvi17
Update src/transformations/correction/geometry_correction.jl
24cd37a 
Minor cleanups
@asinghvi17
Improvements to benchmarks + initial benchmark file (not working)
94c0e78 
Try to fix doc benchmark issue

More benchmark fixes

Solve import error

Add Proj to docs/Project.toml

Add CoordinateTransformations to Docs project.toml

Import CairoMakie into simplify to solve the build issue

Fix simplify execution

Implement doc benchmarks for segmentize
@asinghvi17
Add exception hint for GeodesicSegments
9a751a0
@asinghvi17 asinghvi17 force-pushed the as/correction_and_type_stability branch from 81b127c to 9a751a0 Compare March 22, 2024 21:31
@asinghvi17
Delete correction/valid_point.jl since we aren't using it at this p…
4b2bb6d 
…oint

It needs more optimization as well, which can happen later...
@asinghvi17
Copy link
Member Author

I think I'm pretty much done with what I want to accomplish here! IMO this is mergeable as is - what do you think @rafaqz @skygering?

Change log:

New features

  • Add a geometry correction framework with fix and one method, ClosedRing(), for it. Others can be added in future.
  • Add a segmentize method that corresponds to LibGEOS's density, that upsamples any geometry more complex than a line.
  • Add a benchmark framework which runs quickly and well, and can plot on CI.
    • Implement proof of concept benchmarks for some functions (simplify and segmentize).

Code changes

  • Factor reproject's implementation out to an extension on Proj
    • Provide a nice error hint if Proj isn't loaded and a user tries to use reproject.
  • Since we're getting rid of Proj, also factor out the GeodesicSegments implementation for segementize.
  • Various comment changes and documentation improvements.

@asinghvi17 asinghvi17 merged commit 1e506f0 into main Mar 27, 2024
4 checks passed
@asinghvi17 asinghvi17 deleted the as/correction_and_type_stability branch March 31, 2024 22:47
@asinghvi17 asinghvi17 mentioned this pull request Apr 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaqz rafaqz rafaqz left review comments

@skygering skygering Awaiting requested review from skygering

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asinghvi17 @rafaqz @skygering