Let getnodatavalue return nothing if there is none

[meggart]

Currently the getnodatavalue always returns a value for a RasterBand, no matter if one is actually defined or not. After applying this change, nothing would be returned instead if there is no such value defined. I think although this would be breaking, it would enable a more fine-grained control for handling data with unknown content.

As an alternative and to not break existing code, one could add a hasnodatavalue function instead. I would like to hear other opinions, maybe @rafaqz or @evetion could give a hint how it would affect GeoData.jl and GeoArrays.jl?

I would still need to fix the unit tests but wanted to hear some general opinions first.

[rafaqz]

I fully support this. Currently GeoData.jl throws a warning when it doesnt match the type of the array, but otherwise just assumes its the real nodata value. nothing is much better. This wouldnt might not break GeoData.jl, but would let me remove the annoying warning.

It would need a breaking version bump.

[visr]

If we will deviate from the GDAL behavior here anyway, should we also apply scale and offset if present?

https://gdal.org/api/gdalrasterband_cpp.html#classGDALRasterBand_1abf5ddfd58352f8b2f98af8ab48786d9c

The no data value returned is ‘raw’, meaning that it has no offset and scale applied.

[evetion]

Well I do agree that returning nothing seems better, shouldn't we check the flags from gdalgetmaskflags and not rely on an isnothing check? Only if the flags contain GMF_NODATA, a simple nodata value seems valid (mask/alpha/all-valid being the other flags). We could upstream the logic of these flags from GeoArrays.jl.

Applying scaling/offset to the nodata value seems inconsistent if we also don't do it for retrieving actual the band values, which would be a much larger change (and possible interfere with DiskArrays?). Besides, I have encountered only encountered these scaled/offsetted rasters once or twice.

[meggart]

Well I do agree that returning nothing seems better, shouldn't we check the flags from gdalgetmaskflags and not rely on an isnothing check?

Ah I did not know about gdalgetmaskflags, thanks for the hint. Just to make sure I understand correctly: if the GMF_NODATA bit is set then missing values are encoded using a nodata value and gdalgetrasternodatavalue always succeeds in reading a value?

I think it might be good to expose this functionality in ArchGDAL instead, maybe through simple has_nodatavalue and has_maskband functions that one would check before trying to read the nodata value.

So I would be in favor of closing this one...

[evetion]

Just to make sure I understand correctly: if the GMF_NODATA bit is set then missing values are encoded using a nodata value and gdalgetrasternodatavalue always succeeds in reading a value?

This would be my understanding yes. See the documentation of the flags here: https://gdal.org/doxygen/classGDALRasterBand.html#a181a931c6ecbdd8c84c5478e4aa48aaf

So I would be in favor of closing this one...

We can do both? We should make users aware of the multiple "nodata" options possible, but I still like this improvement of returning nothing instead of a random float.

[meggart]

Ok, then let's continue with this. I have updated the unit tests. I realized that maskflags is already wrapped by ArchGDAL but is not so easily accessible, because it has these bit flags. Would it be in the scope of this package to define an accesible version here, similar to your GeoArrays version? Like:

"""
    maskflaginfo(band::AbstractRasterBand)

Returns the flags as in `maskflags`(@ref) but unpacks the bit values into a named
tuple with the following fields:

* `all_valid`
* `per_dataset`
* `alpha`
* `nodata`

### Returns

A named tuple with unpacked mask flags
"""
function maskflaginfo(band::AbstractRasterBand)
    flags = maskflags(band)
    (
        all_valid = !iszero(flags & 0x01), 
        per_dataset = !iszero(flags & 0x02),
        alpha = !iszero(flags & 0x04),
        nodata = !iszero(flags & 0x08),
    )
end

An alternative would be to use (BitFlags.jl)[https://github.com/jmert/BitFlags.jl] to get pretty printing for which flags are set.

[yeesian]

Would it be in the scope of this package to define an accessible version here, similar to your GeoArrays version?

Yup! We have for e.g.

ArchGDAL.jl/src/driver.jl

Lines 142 to 160 in f6f44ca

	"""
	extensions()
	Returns a `Dict{String,String}` of all of the file extensions that can be read by GDAL,
	with their respective drivers shortname.
	"""
	function extensions()
	extdict = Dict{String,String}()
	for i in 1:ndriver()
	driver = getdriver(i)
	if !(driver.ptr == C_NULL)
	# exts is a space-delimited list in a String, so split it
	for ext in split(metadataitem(driver, "DMD_EXTENSIONS"))
	extdict[".$ext"] = shortname(driver)
	end
	end
	end
	return extdict
	end

[yeesian]

If we will deviate from the GDAL behavior here anyway, should we also apply scale and offset if present?

Correct me if I'm wrong, but I think we're following Julia's conventions, while still remaining consistent with GDAL semantics here? I.e. the current codebase is hardcoding a NULL (where it should really have been handling pbSuccess), and this PR is fixing that behavior.

Applying scale and offset, on the other hand, would result in behavior that's different in semantics from GDAL.

[meggart]

Thanks a lot for the comments from everyone. So we agree, that we should not apply scale_factor and add_offset. I have added the maskflaginfo convenience function to allow users getting detailed information about the applied mask, from my side this would be ready to be merged.

[yeesian]

I have just one remaining (optional) nit, but LGTM otherwise!