mask fails to update existing missing values when missingval is set

[JoshuaBillson]

It seems that when mask is called with the missingval keyword set to a new value, it fails to update the existing missing values in the masked array. For 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=5.0)
2×2 Raster{Float64,2} with dimensions: X, Y
extent: Extent(X = (1, 2), Y = (1, 2))
missingval: 5.0
parent:
 1.0  0.0
 1.0  5.0

We can see that the top-right value is now considered non-missing. I think it's fair to say that this behaviour would be unexpected for most users.

I propose the following solution:

function _mask!(A::RasterStack, with::AbstractRaster; kw...)
    return _mask!(A, boolmask(with); kw...)
end

 function _mask!(A::AbstractRaster, with::AbstractRaster{Bool}; missingval=missingval(A), values=A)
    missingval isa Nothing && _nomissingerror()
    missingval = convert(eltype(A), missingval)

    # Add missing values of A to the mask if the missingval has changed
    bmask = missingval != Rasters.missingval(A) ? boolmask(A) .&& with : with

    broadcast_dims!(A, values, bmask) do x, w
        if isequal(w, Rasters.missingval(bmask))
            missingval
        else
            convert(eltype(A), x)
        end
    end
    return A
end

This solution should cause no changes when missingval is either unspecified or set to the same value. Otherwise, we add the missing values from A to the mask before masking. Because mask relies on _mask!, this should fix the problem for both mask and mask!.

[rafaqz]

That makes sense to me. You could also use boolmask! to avoid the allocation

[JoshuaBillson]

boolmask! seems to be giving me unexpected results. In particular, it sets missing values to non-missing in the dest array.

julia> with = boolmask(b)
2×2 Raster{Bool,2} with dimensions: X, Y
extent: Extent(X = (1, 2), Y = (1, 2))
missingval: false
parent:
 1  1
 1  0

julia> Rasters.boolmask!(with, a)
2×2 Raster{Bool,2} with dimensions: X, Y
extent: Extent(X = (1, 2), Y = (1, 2))
missingval: false
parent:
 1  0
 1  1

If I understand this correctly, it looks like boolmask! simply sets a value in dest to false if it's missing in src, and true otherwise, even if it's missing in dest. Is this intentional?

function boolmask!(dest::AbstractRaster, src::AbstractRaster; missingval=_missingval_or_missing(src))
    broadcast_dims!(dest, src) do a
        !isequal(a, missingval)
    end
end

[JoshuaBillson]

I think this solution would work as well and has the advantage of avoiding additional allocations:

function _mask!(A::AbstractRaster, with::AbstractRaster; missingval=missingval(A), values=A)
    missingval isa Nothing && _nomissingerror()
    missingval = convert(eltype(A), missingval)

    broadcast_dims!(A, values, with) do x, w
        if isequal(w, Rasters.missingval(with)) || isequal(x, Rasters.missingval(values))
            missingval
        else
            convert(eltype(A), x)
        end
    end
    return rebuild(A, missingval=missingval)
end

This is the results of running in the REPL:

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, b, missingval=5)
2×2 Raster{Float64,2} with dimensions: X, Y
extent: Extent(X = (1, 2), Y = (1, 2))
missingval: 5.0
parent:
 1.0  5.0
 1.0  5.0

[rafaqz]

Oh right yes that is intentional for boolmask to work.

Yes was going to say you could just do the broadcast manually its so little code.

That looks ready for a PR if you can, with a test for the mixed missingval case

[JoshuaBillson]

Cool, I'll open a PR with the proposed changes and additional test cases.