Adding mask to open_rasterio #2618

jsignell · 2018-12-18T22:24:04Z

Discussed in Rasterio missing _FillValue in DataArray #1736
Tests added
Fully documented, including whats-new.rst for all changes and api.rst for new API

Not sure if this is the right approach @snowman2

pep8speaks · 2018-12-18T22:24:07Z

Hello @jsignell! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file xarray/tests/test_backends.py:

Line 3138:80: E501 line too long (81 > 79 characters)

Comment last updated at 2019-04-26 15:27:42 UTC

snowman2 · 2018-12-18T22:54:44Z

@jsignell, this looks like it would work - I like the implementation. Some things I would recommend to add is checks for when the nodata value varies across bands (maybe skip the mask and raise a warning) and when the nodata value is already NaN. Also, you should probably set the nodatavals to NaN if masked to resemble the behavior of xarray elsewhere.

I think @fmaussion would be a good reviewer for this PR.

shoyer

Should mask=True to be the default? That's how we handle netCDF files.

shoyer · 2018-12-19T04:03:02Z

xarray/backends/rasterio_.py

@@ -298,6 +300,10 @@ def open_rasterio(filename, parse_coordinates=None, chunks=None, cache=None,
    result = DataArray(data=data, dims=('band', 'y', 'x'),
                       coords=coords, attrs=attrs)

+    if mask:
+        for nodataval in attrs.get('nodatavals', ()):
+            result = result.where(result != nodataval)


The downside of this approach is that unlike how to decode metadata for netCDF files, this won't be lazy.

See CFMaskCoder in xarray/coding/variables.py for who we do this with netCDF files. In particular, it probably makes sense to use lazy_elemwise_func and to move nodatavals from attrs to encoding.

fmaussion · 2018-12-19T10:15:33Z

I wonder if nodatavals is the only attribute for masking - in the doc (https://rasterio.readthedocs.io/en/latest/topics/masks.html) they also mention nodata, which in my experience is also used

snowman2 · 2018-12-19T15:08:34Z

I think the solution would be to add a masked argument and property to the class RasterioArrayWrapper here.

And pass in the (masked=masked) argument here

That would pass riods.read( ..., masked=self.masked) when reading data from rasterio here

jsignell · 2018-12-19T15:11:49Z

I was wondering if we should just pass the mask option to rasterio like that. I'll try it out.

jsignell · 2018-12-19T15:24:28Z

Hmm. Do we really want a np.masked_array or do we just want an array with np.Nans in it? I prefer nans, but I guess the downside is type conversion for ints?

jsignell · 2018-12-19T15:45:04Z

I did as @snowman2 suggested but filled the mask with nans at the end. Since it is now using rasterio directly this should address @fmaussion's concern about nodata (which I think was ending up in the nodatavals attr anyways). I also moved the nodatavals from attrs to encoding when mask==True, but I didn't set it to true by default since I didn't want to break backwards compatibility.

snowman2 · 2018-12-19T16:00:31Z

xarray/backends/rasterio_.py

            np.nan if nodataval is None else nodataval
            for nodataval in riods.nodatavals)
+        if mask:
+            encoding['nodatavals'] = nodatavals


I thing setting the encoding should inline with the other if statements and should be something like:

if mask and riods.nodata is not None: encoding['_FillValue'] = riods.nodata

@shoyer is this what you intended or did you want to keep it as @jsignell modified it to be?

fmaussion · 2018-12-20T17:04:30Z

Do we really want a np.masked_array or do we just want an array with np.Nans in it? I prefer nans, but I guess the downside is type conversion for ints?

Masked arrays come with huge performance drops, I don't think we should use these.

shoyer · 2018-12-20T17:13:19Z

xarray will autoconvert a masked array into a numpy array with NaNs. It probably doesn't make a difference where this happens.

…

On Thu, Dec 20, 2018 at 9:04 AM Fabien Maussion ***@***.***> wrote: Do we really want a np.masked_array or do we just want an array with np.Nans in it? I prefer nans, but I guess the downside is type conversion for ints? Masked arrays come with huge performance drops, I don't think we should use these. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2618 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKS1uztASAzUF-7fNanXXJvRWxELml3ks5u68MegaJpZM4ZZMJn> .

fmaussion · 2019-04-26T10:02:26Z

I'm sorry this has stalled now - I think this is a nice addition.

@jsignell : would you have time to finish this up?

jsignell · 2019-04-26T12:56:10Z

Yeah I have some time to finish it up. Looking back at it now it seems like masked would be a better kwarg for open_rasterio. Is there anything else that should be changed?

snowman2 · 2019-04-26T13:08:39Z

I think it would be good to write the dataset to a netcdf, and read it back in with xarray.open_dataset to ensure everything encodes/decides properly.

jsignell · 2019-04-26T13:15:40Z

I think it would be good to write the dataset to a netcdf, and read it back in with xarray.open_dataset to ensure everything encodes/decides properly.

Do you mean write a test to do this, or just check it locally?

snowman2 · 2019-04-26T13:38:13Z

Good question, I was thinking adding tests.

snowman2 · 2019-07-24T13:01:12Z

xarray/backends/rasterio_.py

@@ -39,6 +39,7 @@ def __init__(self, manager, lock, vrt_params=None):
        if not np.all(np.asarray(dtypes) == dtypes[0]):
            raise ValueError('All bands should have the same dtype')
        self._dtype = np.dtype(dtypes[0])


Suggested change

self._dtype = np.dtype(dtypes[0])

self._dtype = np.float64 if self.masked else np.dtype(dtypes[0])

snowman2 · 2019-07-24T13:02:24Z

xarray/backends/rasterio_.py

-                out = riods.read(band_key, window=window)
+                out = riods.read(band_key, window=window, masked=self.masked)
+                if self.masked:
+                    out = np.ma.filled(out, np.nan)


Suggested change

out = np.ma.filled(out, np.nan)

out = np.ma.filled(out.astype(np.float64), np.nan)

snowman2 · 2019-07-24T13:04:22Z

xarray/backends/rasterio_.py


    # this lets you write arrays loaded with rasterio
    data = indexing.CopyOnWriteArray(data)
    if cache and chunks is None:
        data = indexing.MemoryCachedArray(data)

    result = DataArray(data=data, dims=('band', 'y', 'x'),
-                       coords=coords, attrs=attrs)
+                       coords=coords, attrs=attrs, encoding=encoding)


Suggested change

coords=coords, attrs=attrs, encoding=encoding)

coords=coords, attrs=attrs)

result.encoding = encoding

snowman2 · 2019-11-06T19:37:19Z

@jsignell, the masked kwarg is in rioxarray if you need it: https://corteva.github.io/rioxarray/html/examples/clip_geom.html

keewis · 2021-06-24T08:48:24Z

this has been unintentionally closed when renaming the branch to merge into to main, but given that we consider deprecating open_rasterio in favor of the rasterio engine provided by rioxarray (which seems to already support the mask kwarg) that's probably not a mistake. If you disagree feel free to reopen this PR.

jsignell · 2021-06-24T13:44:33Z

I'm fine with this being closed. TBH I had totally forgotten about it 😬

shoyer reviewed Dec 19, 2018

View reviewed changes

snowman2 reviewed Dec 19, 2018

View reviewed changes

jsignell added 3 commits April 26, 2019 09:08

Fill nodatavals with nan if mask==True

cb9de31

Adding test of mask and default - no masking

8f1fd70

linting

5aa0d4c

jsignell added 2 commits April 26, 2019 09:11

Using rasterio mask on read

66d52c8

Renaming mask -> masked

8b1a263

Adding test to write data to nc and read it back

68bdc7f

snowman2 mentioned this pull request Jun 27, 2019

Add open function. corteva/rioxarray#7

Closed

snowman2 mentioned this pull request Jul 23, 2019

added rioxarray.open_rasterio corteva/rioxarray#31

Merged

3 tasks

snowman2 reviewed Jul 24, 2019

View reviewed changes

andersy005 added the waiting for author label Dec 28, 2020

max-sixty removed the waiting for author label Jun 5, 2021

keewis closed this Jun 23, 2021

keewis deleted the branch pydata:master June 23, 2021 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding mask to open_rasterio #2618

Adding mask to open_rasterio #2618

jsignell commented Dec 18, 2018

pep8speaks commented Dec 18, 2018 •

edited

Loading

snowman2 commented Dec 18, 2018

shoyer left a comment

shoyer Dec 19, 2018

fmaussion commented Dec 19, 2018

snowman2 commented Dec 19, 2018 •

edited

Loading

jsignell commented Dec 19, 2018

jsignell commented Dec 19, 2018

jsignell commented Dec 19, 2018

snowman2 Dec 19, 2018

snowman2 Dec 19, 2018

fmaussion commented Dec 20, 2018

shoyer commented Dec 20, 2018 via email

fmaussion commented Apr 26, 2019

jsignell commented Apr 26, 2019

snowman2 commented Apr 26, 2019

jsignell commented Apr 26, 2019

snowman2 commented Apr 26, 2019

snowman2 Jul 24, 2019 •

edited

Loading

snowman2 Jul 24, 2019

snowman2 Jul 24, 2019 •

edited

Loading

snowman2 commented Nov 6, 2019

keewis commented Jun 24, 2021

jsignell commented Jun 24, 2021

	self._dtype = np.dtype(dtypes[0])
	self._dtype = np.float64 if self.masked else np.dtype(dtypes[0])

	out = np.ma.filled(out, np.nan)
	out = np.ma.filled(out.astype(np.float64), np.nan)

	coords=coords, attrs=attrs, encoding=encoding)
	coords=coords, attrs=attrs)
	result.encoding = encoding

Adding mask to open_rasterio #2618

Adding mask to open_rasterio #2618

Conversation

jsignell commented Dec 18, 2018

pep8speaks commented Dec 18, 2018 • edited Loading

Comment last updated at 2019-04-26 15:27:42 UTC

snowman2 commented Dec 18, 2018

shoyer left a comment

Choose a reason for hiding this comment

shoyer Dec 19, 2018

Choose a reason for hiding this comment

fmaussion commented Dec 19, 2018

snowman2 commented Dec 19, 2018 • edited Loading

jsignell commented Dec 19, 2018

jsignell commented Dec 19, 2018

jsignell commented Dec 19, 2018

snowman2 Dec 19, 2018

Choose a reason for hiding this comment

snowman2 Dec 19, 2018

Choose a reason for hiding this comment

fmaussion commented Dec 20, 2018

shoyer commented Dec 20, 2018 via email

fmaussion commented Apr 26, 2019

jsignell commented Apr 26, 2019

snowman2 commented Apr 26, 2019

jsignell commented Apr 26, 2019

snowman2 commented Apr 26, 2019

snowman2 Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

snowman2 Jul 24, 2019

Choose a reason for hiding this comment

snowman2 Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

snowman2 commented Nov 6, 2019

keewis commented Jun 24, 2021

jsignell commented Jun 24, 2021

pep8speaks commented Dec 18, 2018 •

edited

Loading

snowman2 commented Dec 19, 2018 •

edited

Loading

snowman2 Jul 24, 2019 •

edited

Loading

snowman2 Jul 24, 2019 •

edited

Loading