Fix _extent when called on an extent
#852
Conversation
|
We most definitely do need some tests! Can't understand how this one slipped through |
|
Want to add a test now? |
| @@ -50,6 +50,7 @@ function _extent(::GI.AbstractFeatureCollectionTrait, features; kw...)::XYExtent | |||
| end | |||
| return _float64_xy_extent(ext) | |||
| end | |||
| _extent(ext::Extent; kw...) = _float64_xy_extent(ext) | |||
There was a problem hiding this comment.
I thought about this one, but do we really want to force the convert to float64? This will mean that rasterize with an extent in integers or float32 will return float64 dimensions
There was a problem hiding this comment.
We convert everything else already. I think there were 2 reasons - accuracy of ext + res/size is much better with Float64, and it reduces compilation for force the extent to always be the same object.
But we can change everything back to allow Float32 in another PR if you think its necessary
There was a problem hiding this comment.
I think we want _extent(ext::Extent; kw...) = Extents.Extent(X=ext.X, Y=ext.Y). If ext doesn't have an X or Y field it will give a reasonable error message.
There was a problem hiding this comment.
Okay if it gets converted elsewhere then nevermind
There was a problem hiding this comment.
We actually want to force ::XYExtent on the method to be consistent
There was a problem hiding this comment.
We can have the Float32 converstion elsewhere but lets conform with the current setup for now
There was a problem hiding this comment.
Yeah, but without that my actual example failed when I used a Float32 extent.
There was a problem hiding this comment.
Just checked and I don't think this gets converted anywhere else, though. With the above definition of _extent I can rasterize to Float32 dimensions no problem. I haven't run into this yet, but don't see why we shouldn't allow it.
There was a problem hiding this comment.
Okay I didn't realize XYExtent has the Float64 type hard-coded in. Guess that is needed for type stability?
There was a problem hiding this comment.
I think type stability was one reason, this recursive calls were not compiling away and forcing the type fixes that
Co-authored-by: Rafael Schouten <[email protected]>
| @@ -762,3 +762,10 @@ test = rebuild(ga; name = :test) | |||
| @test_throws "strictly positive" Rasters.sample(StableRNG(123), test, 3, skipmissing = true, replace = false) | |||
| @test_throws "Cannot draw" Rasters.sample(StableRNG(123), test, 5, replace = false) | |||
| end | |||
|
|
|||
| @testset "extent" begin | |||
| ga = Raster(A, (X(1.0:1:2.0), Y(1.0:1:2.0)); missingval=missing) | |||
There was a problem hiding this comment.
We should also test this for an Extent, that is based on float32
There was a problem hiding this comment.
@tiemvanderdeure we need this to merge
There was a problem hiding this comment.
I thought you said you wanted to fix that in a separate PR? Right now this would fail, I guess?
There was a problem hiding this comment.
Will it? I don't totally understand, won't it just convert to Float64?
We just need to check the input works
There was a problem hiding this comment.
What I meant is the Rasters._extent(ext) === ext will fail if ext is float32-based. But I've added some tests anyway
|
Just noticed there were a few places where keywords (specifically EDIT: Sorry for squeezing another commit in, but I noticed that there was still a bunch of code that does what |
|
We just need #853 to merge to address the test failure. |
|
Tests pass, I think this is ready to merge |
Solves #851