-
Notifications
You must be signed in to change notification settings - Fork 5
Algorithm + manifold and accelerator passthrough in clipping functions #273
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
Conversation
60eabb9
to
b52ef6c
Compare
941f27d
to
bc9a491
Compare
b52ef6c
to
f2d90a8
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but have to remove line breaks between methods of the same function 😆
struct DoubleSTRtree <: IntersectionAccelerator end | ||
struct SingleNaturalTree <: IntersectionAccelerator end | ||
struct DoubleNaturalTree <: IntersectionAccelerator end | ||
struct ThinnedDoubleNaturalTree <: IntersectionAccelerator end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only one with thinning?
Wondering if it can be a type parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we could construct a thinned str tree as well, or some other tree sorting alg for RTrees.
In all honesty the thinning ones will only get used when you're constructing a tree from scratch. Otherwise (in the ideal world) AutoAccelerator will pick up on it and force that particular tree / pattern to be used unless it's really suboptimal.
f2d90a8
to
974a162
Compare
974a162
to
5b0c4a1
Compare
5b0c4a1
to
c36826d
Compare
This was factored out of the "dev branch" #259 and contains the subset of changes that apply to GeometryOpsCore, for easier review. Child PRs: #271 (TGGeometry) -> #275 (AdaptivePredicates) -> #273 (clipping algorithm type) -> #274 (trees) - Use [StableTasks.jl](https://github.com/JuliaFolds2/StableTasks.jl) in apply and applyreduce - its type-stable tasks save us some allocations! - Remove `Base.@assume_effects` on the low level functions, which caused issues on Julia v1.11 and was probably incorrect anyway - Add an algorithm interface with an abstract supertype `Algorithm{M <: Manifold}`, as discussed in #247. Also adds an abstract Operator supertype and some discussion in code comments, but no implementation or interface surface there yet. - Split out `types.jl` into a directory `types` with a bunch of files in it, for ease of readability / docs / use. - (out of context change): refactor CI a bit for cleanliness. TODOs for later (not this PR): - [ ] Add a `format` method that takes in an incompletely specified algorithm and some geometry as input, and returns a completely specified algorithm. What does this mean? Imagine I call `GO.intersection(FosterHormannClipping(), geom1, geom2)`. That `FosterHormannClipping()` should get expanded to `FosterHormannClipping(AutoAlgorithm(), AutoAccelerator())`. Then, `format` will take `format(alg, args...)` and: - get the `crstrait` of the two geometries, scan for incompatibilities, assign the correct manifold to the algorithm (maybe warn or emit debug info) - if no geometries available, get the manifold via `best_manifold(::Algorithm)`. - maybe inflate the accelerator by checking `npoint` and later preparations to see what's most efficient, maybe not - depends on what we want!
c36826d
to
62187d6
Compare
a5377d4
to
b3b8d4b
Compare
62187d6
to
e29badb
Compare
23080c1
to
9e50abe
Compare
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
Maybe this is a bad idea, single file was nice for readability. But we can keep going.
1842fbf
to
d5a49de
Compare
Not sure of name (too verbose, maybe Foster()?). Intersection accelerators will come into the picture later (next PR) but we need them now to instantiate the struct.
Better than printing out since we can (A) check on it and (B) access the polygons as well as the a_list and b_list in Julia. I have a few plot recipes for them but not sure if they are worth putting in a Makie extension...they would also be inaccessible there. Maybe in GeoMakie? Then we can make full use of customizations etc.
This is a shorter change list as a sampler, to show what needs to happen.
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
I'm OK with coverage dropping a bit here because a lot of those methods only make sense to test once we've implemented the operation machinery. |
Yeah, looks good to merge to me |
Spun out from #259
Parent: #275 (adaptive predicates)
Child PRs: #274 (trees)
FosterHormannClipping(manifold, accelerator)
algorithm - we can bikeshed the nameTracingError
that actually stores all the context including a_list and b_list, this way we can more effectively diagnose what's going on.