Coverage #60
Coverage #60
Conversation
|
IMO coverage should probably be a separate file here? |
|
|
How many vertices does |
|
Could be good to accept an |
Oh! That is a great idea. Then I don't need the point_in_cell functions! I will work swapping that! |
Not many. Like 10 or something. I deal with pretty small polygons (at least small numbers of points, if not area) most of the time so I don't have good "large" polygons to test on. If you have some and open a PR to put them in the test folder as an input that would be super cool. |
@rafaqz In hindsight, I don't know if this is a good idea. On one hand, it would be a nicer input. On the other hand, we now have to make Extent objects, which is very, very cheap, but it still costs something. And if folks are dealing with cells, they would now need to make an Extent object for every cell. Thoughts? If we use: function f1(xmin, xmax, ymin, ymax, p)
p_ext = GI.extent(p)
cell_ext = Extents.Extent(X = (xmin, xmax), Y = (ymin, ymax))
return Extents.intersects(p_ext, cell_ext)
end
f2(xmin, xmax, ymin, ymax, p) = xmin ≤ GI.x(p) ≤ xmax && ymin ≤ GI.y(p) ≤ ymaxthen we get: |
|
It should cost nothing at all unless you have mixed types somehow? They will promote. Otherwise its a bug. I actually just meant we should let users pass in an extent if they want to |
|
Oh actually by making the point an extent you are doubling the number of comparisons!! |
|
Okay, I moved the code and I also used But I feel good other than that. It can also take in an If you guys think it is ready to go then I will merge. |
|
Whatever happens we will make |
| applyreduce(+, _COVERAGE_TARGETS, geom; threaded, init=zero(T)) do g | ||
| _coverage(T, GI.trait(g), g, T(xmin), T(xmax), T(ymin), T(ymax)) | ||
| end |
There was a problem hiding this comment.
This is the only thing I'm not sure about, since the function is inside the applyreduce call. Still, it will operate at a pretty high level and hopefully that shouldn't cause too many issues.
A potential solution might be to accept a tuple (xmin, xmax, ymin, ymax) in _coverage, and then just use Base.Fix2 to create a function.
There was a problem hiding this comment.
Giving the anonymous function a name just on the line above can help the compiler. Surprisingly.
All the T types we are using everywhere worry me a but too, the compiler likes to forget what they are... sometimes it can help to wrap them in Val further out to force them through
There was a problem hiding this comment.
I don't think I understand the problem. Can you elaborate? Is this something we need to fix pre-merge?
There was a problem hiding this comment.
This would be:
| applyreduce(+, _COVERAGE_TARGETS, geom; threaded, init=zero(T)) do g | |
| _coverage(T, GI.trait(g), g, T(xmin), T(xmax), T(ymin), T(ymax)) | |
| end | |
| coverage_for_subgeom(g) = _coverage(T, GI.trait(g), g, T(xmin), T(xmax), T(ymin), T(ymax)) | |
| applyreduce(coverage_for_subgeom, +, _COVERAGE_TARGETS, geom; threaded, init=zero(T)) |
which gets rid of the anonymous function created by the do block.
There was a problem hiding this comment.
Should I change this in area as well then, since I copied this format from there?
|
I can merge this now and we'll get to the optimization in #69 |
This calculates the area of a geometry within a bounding box defined by the minimum and maximum x and y values. So this should be similar to
coveragewithin rasters, just for one polygon at a time.I haven't tested, but right now for multi-geometries and collections, I think it would be equivalent to Raster's
coveragewith the sum operation.I am not sure if
area.jlis the right place for this or if it should have its own file...