refactor: nodata handling #162 #163

Kirill888 · 2024-06-18T06:38:11Z

adding new types
- ~~MaybeAutoNodata~~ SomeNodata -- None|int|float|str|"auto"
- Nodata is now None|float|int
- MaybeNodata is now ~~None|float|int|str~~ have been removed
- "auto" replaces what used to be None
- None now means "no nodata value"
resolve_nodata() is used to handle nodata options consistently across the library
.odc.nodata is used to extract nodata from xarrays
fixes in reproject/overview generation for float data. It is assumed to have nan values, and GDAL needs nodata=nan to handle it correctly.

nodata types explained

~~MaybeAutoNodata~~SomeNodata should be used in APIs that allow configuring nodata and nodata overrides
Nodata is a type suitable to pass to GDAL {src,dst)_nodata it can be None which means expect no missing values in the data.
FillValue is a type suitable for np.full and is resolved from Nodata + dtype
- NaN for not-configured floating point images
- 0 for not-configured integer images
- value of nodata if set (NaN is fine here too)

SomeNodata + dtype + [fallback] -> Nodata + dtype -> FillValue

github-actions · 2024-06-18T06:40:32Z

🚀 Deployed on https://667240bc235f9b98b2a5c58c--odc-geo-docs.netlify.app

- adding new types - `MaybeAutoNodata` -- `None|int|float|str|"auto"` - `Nodata` is now `None|float|int` - `MaybeNodata` is now `None|float|int|str - "auto" replaces what used to be `None` - `None` now means "no nodata value" - `resolve_nodata()` is used to handle nodata options consistently across the library - default nodata for float is `nan` - fixes in reproject/overview generation for float data. It is assumed to have `nan` values, and GDAL needs `nodata=nan` to handle it correctly.

SpacemanPaul · 2024-06-18T06:56:28Z

Thanks for the swift action on this Kirill - I will review tomorrow.

- bump versions for netlify - supply token to codecov

codecov · 2024-06-18T07:00:18Z

Codecov Report

Attention: Patch coverage is 98.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.48%. Comparing base (6af5d0c) to head (e80f39a).
Report is 33 commits behind head on develop.

Files	Patch %	Lines
odc/geo/_xr_interop.py	96.55%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #163      +/-   ##
===========================================
+ Coverage    95.26%   95.48%   +0.21%     
===========================================
  Files           31       31              
  Lines         5323     5489     +166     
===========================================
+ Hits          5071     5241     +170     
+ Misses         252      248       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

robbibt · 2024-06-18T23:54:26Z

Thanks heaps for this @Kirill888 - the test cases appear to match the functionality I was hoping for nicely, but I'll do a "user" test today and verify that TIFFs exported using the updated code work as intended (e.g. in ESRI etc).

Kirill888 · 2024-06-19T00:30:39Z

@robbibt thanks, BTW pleas use pip install odc-geo==0.4.7rc1 for your test (that release is not on conda, only pypi)

robbibt · 2024-06-19T00:30:41Z

I think this is working perfectly. For example, we have a dataset with an Xarray nodata: nan attribute:

import datacube
import odc.geo.xr
from datacube.utils.cog import write_cog

dc = datacube.Datacube()

query_params = dict(
    x=(142.13223, 142.65461),
    y=(-32.17591, -32.54618),
    time=("2022", "2022"),
)

ds = dc.load(product="ga_ls8cls9c_gm_cyear_3", measurements=["edev"], **query_params)
da = ds.edev.squeeze()

This gets written out with a GeoTIFF nodata flag nodata=nan using both datacube and odc-geo tooling:

da.odc.write_cog("nodata_nan_odcgeo.tif")
write_cog(da, "nodata_nan_datacube.tif")

However, if we write out data without a Xarray nodata: nan attribute, datacube doesn't include a GeoTIFF nodata flag, but now odc-geo does!

del da.attrs["nodata"]

da.odc.write_cog("nodata_missing_odcgeo.tif")
write_cog(da, "nodata_missing_datacube.tif")

I can though return a true missing nodata value like this:

da.odc.write_cog("nodata_truenone_odcgeo.tif", nodata=None)

SpacemanPaul · 2024-06-19T01:12:42Z

odc/geo/types.py

+FillValue = Union[float, int]
+Nodata = Union[float, int, None]
+MaybeNodata = Union[float, int, str, None]
+MaybeAutoNodata = Union[float, int, str, None, Literal["auto"]]
 T = TypeVar("T")


MaybeNodata = Nodata | str seems unexpected as Maybe usually means or None. I understand that str might be necessary for cases where nodata in ('nan', 'NAN', 'NaN') but I'm not sure why this gets a "Maybe" label?

Oh I see, there's a conversion function in math.py

I still think the naming here is confusing but I don't have a good suggestion for a replacement. Maybe ApiNodata? As in the nodata type the user sees in the API (as opposed the internal nodata value passed to GDAL)?

that's more of a compatibility issue, before this Nodata could not be None, but it could be a str, so there was a MaybeNodata also. I guess we could delete it assuming nobody was using it.

SpacemanPaul

I think some of the naming is confusing, but the approach is sound.

SpacemanPaul · 2024-06-19T01:15:42Z

odc/geo/types.py

+FillValue = Union[float, int]
+Nodata = Union[float, int, None]
+MaybeNodata = Union[float, int, str, None]
+MaybeAutoNodata = Union[float, int, str, None, Literal["auto"]]
 T = TypeVar("T")


Oh I see, there's a conversion function in math.py

I still think the naming here is confusing but I don't have a good suggestion for a replacement. Maybe ApiNodata? As in the nodata type the user sees in the API (as opposed the internal nodata value passed to GDAL)?

robbibt · 2024-06-19T01:28:03Z

I do want to do a quick test of the COG overviews stuff, so will approve this as soon as I've finished that.

robbibt · 2024-06-19T01:28:57Z

I think some of the naming is confusing, but the approach is sound.

I also find "MaybeAutoNodata" a bit confusing - took me a while to work out what it meant.

Kirill888 · 2024-06-19T01:57:26Z

I think some of the naming is confusing, but the approach is sound.

I also find "MaybeAutoNodata" a bit confusing - took me a while to work out what it meant.

What about SomeNodata instead of MaybeAutoNodata, @SpacemanPaul @robbibt

SpacemanPaul · 2024-06-19T02:07:32Z

I think some of the naming is confusing, but the approach is sound.

I also find "MaybeAutoNodata" a bit confusing - took me a while to work out what it meant.

What about SomeNodata instead of MaybeAutoNodata, @SpacemanPaul @robbibt

Yes SomeNodata or even AnyNodata works for me.

now that `Nodata` is allowed to be `None`, 'MaybeNodata' is now confusing, removing it. Let's assume that it never was a part of the `odc.geo` API.

Kirill888 · 2024-06-19T02:21:38Z

Updated,

Removed no longer relevant MaybeNodata type, if anyone was using that type, they were probably also using Nodata type too, and that have changed, so...
use SomeNodata instead of MaybeAutoNodata name
add nodata setter

robbibt · 2024-06-19T02:49:38Z

OK, overview generation is working much better for our DEA Intertidal example: even if nodata is not set, we still get a result that follows GDAL's expected functionality:

datacube-core with nodata=nan Xarray attribute (overviews generated correctly ✔️):

datacube-core with no nodata Xarray attribute (overviews generated incorrectly ❌):

odc-geo with nodata=nan Xarray attribute (overviews generated correctly ✔️):

odc-geo with no nodata Xarray attribute (overviews generated correctly ✔️):

Kirill888 requested review from robbibt and SpacemanPaul June 18, 2024 06:38

github-actions bot temporarily deployed to pull request June 18, 2024 06:42 Inactive

Kirill888 force-pushed the fix-float-nodata branch from 58b828c to fc2ea18 Compare June 18, 2024 06:45

github-actions bot temporarily deployed to pull request June 18, 2024 06:48 Inactive

Update GH actions

44c956a

- bump versions for netlify - supply token to codecov

github-actions bot temporarily deployed to pull request June 18, 2024 07:00 Inactive

github-actions bot temporarily deployed to pull request June 18, 2024 07:17 Inactive

Kirill888 added 2 commits June 18, 2024 17:24

Expand coverage for resolve_nodata method

ee3297b

Release candidate v0.4.7rc1

b9aa246

Kirill888 force-pushed the fix-float-nodata branch from 483fd09 to b9aa246 Compare June 18, 2024 07:24

github-actions bot temporarily deployed to pull request June 18, 2024 07:26 Inactive

SpacemanPaul reviewed Jun 19, 2024

View reviewed changes

SpacemanPaul approved these changes Jun 19, 2024

View reviewed changes

Add .odc.nodata = setter

ca1d3d0

Kirill888 added 2 commits June 19, 2024 12:13

fix: PR feedback change type names

1cce9e8

fix: remove no longer relevant MaybeNodata type

e80f39a

now that `Nodata` is allowed to be `None`, 'MaybeNodata' is now confusing, removing it. Let's assume that it never was a part of the `odc.geo` API.

Kirill888 requested a review from SpacemanPaul June 19, 2024 02:21

github-actions bot temporarily deployed to pull request June 19, 2024 02:21 Inactive

Kirill888 merged commit 78577dc into develop Jun 19, 2024
20 checks passed

Kirill888 deleted the fix-float-nodata branch June 19, 2024 02:52

Kirill888 mentioned this pull request Jun 19, 2024

Investigate float image nan support #162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: nodata handling #162 #163

refactor: nodata handling #162 #163

Kirill888 commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading

SpacemanPaul commented Jun 18, 2024

codecov bot commented Jun 18, 2024 •

edited

Loading

robbibt commented Jun 18, 2024

Kirill888 commented Jun 19, 2024

robbibt commented Jun 19, 2024

SpacemanPaul Jun 19, 2024

SpacemanPaul Jun 19, 2024

Kirill888 Jun 19, 2024

SpacemanPaul left a comment

SpacemanPaul Jun 19, 2024

robbibt commented Jun 19, 2024

robbibt commented Jun 19, 2024

Kirill888 commented Jun 19, 2024

SpacemanPaul commented Jun 19, 2024

Kirill888 commented Jun 19, 2024 •

edited

Loading

robbibt commented Jun 19, 2024

refactor: nodata handling #162 #163

refactor: nodata handling #162 #163

Conversation

Kirill888 commented Jun 18, 2024 • edited Loading

nodata types explained

github-actions bot commented Jun 18, 2024 • edited Loading

SpacemanPaul commented Jun 18, 2024

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

robbibt commented Jun 18, 2024

Kirill888 commented Jun 19, 2024

robbibt commented Jun 19, 2024

SpacemanPaul Jun 19, 2024

Choose a reason for hiding this comment

SpacemanPaul Jun 19, 2024

Choose a reason for hiding this comment

Kirill888 Jun 19, 2024

Choose a reason for hiding this comment

SpacemanPaul left a comment

Choose a reason for hiding this comment

SpacemanPaul Jun 19, 2024

Choose a reason for hiding this comment

robbibt commented Jun 19, 2024

robbibt commented Jun 19, 2024

Kirill888 commented Jun 19, 2024

SpacemanPaul commented Jun 19, 2024

Kirill888 commented Jun 19, 2024 • edited Loading

robbibt commented Jun 19, 2024

Kirill888 commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

Kirill888 commented Jun 19, 2024 •

edited

Loading