actually test embed_extent #29
Conversation
src/transformations/extent.jl
Outdated
| @@ -6,19 +6,24 @@ calculating and adding an `Extents.Extent` to all objects. | |||
|
|
|||
| This can improve performance when extents need to be checked multiple times. | |||
There was a problem hiding this comment.
I think this could use a bit more comments. I wasn't familiar with this concept and it took a good few minutes to understand what was going on. Maybe a brief description of what extents are and how to access them after running.
There was a problem hiding this comment.
Ahh right, I'm assuming a lot of knowledge here. Definately more docs could help!
The ideas is you can use the extent as e.g. a fast pointinpolygon proxy. If its in the extent you need to actually check the polygon, if its not you know in a few ns.
src/transformations/extent.jl
Outdated
| @@ -6,19 +6,24 @@ calculating and adding an `Extents.Extent` to all objects. | |||
|
|
|||
| This can improve performance when extents need to be checked multiple times. | |||
| """ | |||
| embed_extent(x; kw...) = apply(AbstractTrait, x; kw...) | |||
| embed_extent(x; kw...) = apply(extent_applicator, GI.AbstractTrait, x; kw...) | |||
There was a problem hiding this comment.
We also might want to specify here what keywords are possible. Cross-referencing with the with primitives.jl, I can see that it they are: crs, calc_extent, and threaded... however, down below we recalculate crs and we are obviously calculating extent so we might what to specify why we use apply.
There was a problem hiding this comment.
Ahh probably this can all be written just using one line - apply(identity, PointTrait, x; calc_extent=true) !!!
calc_extent already preopagates the extents efficiently.
And then the threaded and crs keywords make sense. We can just block the calc_extent keyword.
lol
| using Test | ||
|
|
||
| import GeoInterface as GI | ||
| import GeometryOps as GO |
There was a problem hiding this comment.
Would be good to test at least one curve or something that isn't nested since that is a separate function
There was a problem hiding this comment.
If we use apply for everything than it also needs less tests.
|
It seems like the |
|
Ok the code is so simple now this PR is more about adding keyword documentation to all the transformations 🤣 I thought I may as well do that systematically if I added it to |
| @@ -1,18 +1,31 @@ | |||
| const THREADED_KEYWORD = "- `threaded`: `true` or `false`. Whether to use multithreading. Defaults to `false`." | |||
There was a problem hiding this comment.
Oh this is such a good way of not having to retype the same documentation over and over. I am so stealing this for my other project 😆
There was a problem hiding this comment.
Yeah, from Rasters.jl experience. Actually the main reason is so users see exactly the same things everywhere and it feels consistent
|
It is looking really good. I think it might be worth it in some PR, even if not this PR, to clean up the |
|
Yeah I'll add that later |
I added this code without finishing or testing it. So this PR does that.
embed_extentembeds anExtents.Extentobject in all geometries recursively. This meansextentcalls are basically free when you need them later.Useful as preparation for algorithms that need to call
Extents.extenton everything.