mask and mask! now remove missing values in the source raster

[JoshuaBillson]

Fixes a bug where calling mask with the missingval keyword set to a new value would fail to update the existing missing values in the masked array.

[JoshuaBillson]

Note: I still need to write test cases.

[codecov-commenter]

Codecov Report

Merging #507 (0c22528) into main (ff90b8f) will increase coverage by 7.38%.
The diff coverage is 100.00%.

❗ Current head 0c22528 differs from pull request most recent head e9a31ba. Consider uploading reports for the commit e9a31ba to get more accurate results

@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   73.06%   80.44%   +7.38%     
==========================================
  Files          59       50       -9     
  Lines        4633     4173     -460     
==========================================
- Hits         3385     3357      -28     
+ Misses       1248      816     -432     

Files	Coverage Δ
src/methods/mask.jl	90.00% <100.00%> (+0.38%)	⬆️

... and 21 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[rafaqz]

Should we just error here instead? If it wont convert it wont be much use as a missingval.

Number wont all work in convert either so that's effectively what happens in the method above anyway.

[JoshuaBillson]

That's a good point about Number. However, even if missingval can't be converted to eltype(A), it can still be used as a valid missing value. Here's an example:

julia> a = Raster([1.0 0.0; 1.0 1.0], dims=(X, Y), missingval=0);

julia> b = Raster([1.0 1.0; 1.0 0.0], dims=(X, Y), missingval=0);

julia> mask(a, with=b, missingval=missing)
2×2 Raster{Union{Missing, Float64},2} with dimensions: X, Y
extent: Extent(X = (1, 2), Y = (1, 2))
missingval: missing
parent:
 1.0  missing
 1.0  missing

julia> convert(eltype(a), missing)
ERROR: MethodError: Cannot `convert` an object of type Missing to an object of type Float64

Closest candidates are:
  convert(::Type{T}, ::ColorTypes.Gray24) where T<:Real
   @ ColorTypes ~/.julia/packages/ColorTypes/1dGw6/src/conversions.jl:114
  convert(::Type{T}, ::ColorTypes.Gray) where T<:Real
   @ ColorTypes ~/.julia/packages/ColorTypes/1dGw6/src/conversions.jl:113
  convert(::Type{T}, ::T) where T<:Number
   @ Base number.jl:6
  ...

As we can see, missingval cannot be converted to eltype(A), which in this case is missing and Float64, respectively. However, broadcast_dims is still able to write it in as a missing value.

[JoshuaBillson]

I did some more digging, and it seems that mask works fine with a missingval that can't be converted to the element type of the destination raster. However, mask! throws an error.

I've narrowed the reason down to this:

function _mask(A::AbstractRaster, with::AbstractRaster;
    filename=nothing, suffix=nothing, missingval=_missingval_or_missing(A), kw...
)
    A1 = create(filename, A; suffix, missingval) # Returns Matrix{Union{Missing, Float64}}
    open(A1; write=true) do a
        # The values array will be be written to A1 in `mask!`
        mask!(a; with, missingval, values=A)
    end
    return A1
end

create seems to create a new array with a Union element type supporting the new missingval. This doesn't occur in mask!, as it doesn't allocate a new array. We can stop mask from allowing non-convertible missing values by placing a missingval = convert(eltype(A), missingval) immediately before we create A1. The question is whether or not this behaviour is desirable.

[rafaqz]

Oh you may be right with convert, I was confused as to where the conversion was happening. I guess putting everything in the try/catch is ok, although its a bit icky doing that.

We could also just say that only convertable values or missing can be used. It doesnt really make sense to use anything but missing or something convertable, as any missingval you can write to file has to be the same type.

And yes, mask! is the in-place allocation free version, so the passed-in array needs to be capable of writing the new missingval.

[rafaqz]

Is this good for review/merge?

[JoshuaBillson]

Is this good for review/merge?

Almost. I need to add some test cases for mask!.

[JoshuaBillson]

This should be good to merge now, but for some reason, the documenter and test cases are timing out when attempting to download external data.

[rafaqz]

Thanks.

The tests are just ucdavis IT. WorldClim is down about 5 days of the year unfortunately.