Modular encodings (rebased) #245

shoyer · 2014-10-02T18:05:50Z

This change is rebased on master and should let us pick up from #175. CC @akleeman

Restructured Backends to make CF convention application more consistent.

Amongst other things this includes:

EncodedDataStores which can wrap other stores and allow
for modular encoding/decoding.
Trivial indices ds['x'] = ('x', np.arange(10)) are no longer
stored on disk and are only created when accessed.
AbstractDataStore API change. Shouldn't effect external users.
missing_value attributes now function like _FillValue

All current tests are passing (though it could use more new ones).

Post rebase notes (shoyer, Oct 2, 2014):
Most tests are passing, though a couple are broken:

test_roundtrip_mask_and_scale (because this change needs a fix to not
break the current API)
test_roundtrip_strings_with_fill_value on TestCFEncodedDataStore
(I don't entirely understand why, let's come back to it later)

consistent. Amongst other things this includes: - EncodedDataStores which can wrap other stores and allow for modular encoding/decoding. - Trivial indices ds['x'] = ('x', np.arange(10)) are no longer stored on disk and are only created when accessed. - AbstractDataStore API change. Shouldn't effect external users. - missing_value attributes now function like _FillValue All current tests are passing (though it could use more new ones). Post rebase notes (shoyer, Oct 2, 2014): Most tests are passing, though a couple are broken: - test_roundtrip_mask_and_scale (because this change needs a fix to not break the current API) - test_roundtrip_strings_with_fill_value on TestCFEncodedDataStore (I don't entirely understand why, let's come back to it later)

shoyer · 2014-10-05T04:24:42Z

closing this for now until the discussion in #175 is resolved.

a model where encoding/decoding happens when a dataset is stored to/ loaded from a DataStore. Conventions can now be enforced at the DataStore level by overwritting the Datastore.store() and Datastore.load() methods, or as an optional arg to Dataset.load_store, Dataset.dump_to_store. Includes miscelanous cleanup.

shoyer · 2014-10-08T20:30:40Z

xray/core/dataset.py

+                                 'supported on Python 2.6')
+            try:
+                store = backends.ScipyDataStore(gzip.open(nc), *args, **kwargs)
+            except TypeError, e:


the more modern syntax is except TypeError as e

shoyer · 2014-10-08T20:43:49Z

Is there a reason you switched to using accessors (e.g., get_variables) instead of properties? Properties feel a little more pythonic to me. I like the idea of making property classes (e.g., DataStoreVariables) that abstract away the set_variable nonsense behind a dictionary like interface. I can submit a patch to add that, if you like.

akleeman · 2014-10-09T01:37:40Z

re: using accessors such as get_variables.

To be fair, this pull request didn't really switch the behavior, it was already very similar but with different names (store_variables, open_store_variable) that were now changed to get_variables etc. I chose to continue with getters and setters because it makes it fairly clear what needs to be implemented to make a new DataStore, and allows for some post processing such as _decode_variable_name and encoding/decoding. Its not entirely clear to me how that would fit into a properties based approach so sounds like a good follow up pull request to me.

shoyer · 2014-10-09T01:53:38Z

K, fine by me.

On Wed, Oct 8, 2014 at 6:37 PM, akleeman [email protected] wrote:

re: using accessors such as get_variables.

To be fair, this pull request didn't really switch the behavior, it was already very similar but with different names (store_variables, open_store_variable) that were now changed to get_variables etc. I chose to continue with getters and setters because it makes it fairly clear what needs to be implemented to make a new DataStore, and allows for some post processing such as _decode_variable_name and encoding/decoding. Its not entirely clear to me how that would fit into a properties based approach so sounds like a good follow up pull request to me.

Reply to this email directly or view it on GitHub:
#245 (comment)

shoyer · 2014-10-09T04:01:22Z

xray/backends/netCDF4_.py

+        self._encoder_args = args
+        self._encoder_kwdargs = kwdargs
+
+    def store(self, variables, attributes):


can we move this method to a mixin class (something like AlwaysWriteCFEncoded) and use multiple inheritance to add it to NetCDF4DataStore and ScipyDataStore?

Doing that would get a little awkward since the encoding is now embedded within a store, so arguments to the encoder are passed into the DataStore constructor. As a result the mixin class would need to implement a constructor which stored arguments which are intended for the encoder, and the DataStore that extends the mixin would need to distinguish between arguments intended for the store/mixin. Doing this in any sort of automated way leads to the nasty bit of logic (encoding_decorator) that I just removed.

One alteration which might make that all less nasty is to have any encoding/decoding arguments be directly passed into DataStore store/load. Though, in the interest of not further bloating this PR I'd like to save that future change.

Yes, I do like the idea of passing encoding/decoding arguments directly into store/load.

On second thought, I do agree with you -- this is not enough redundant code to worry about factoring out.

shoyer · 2014-10-09T18:43:13Z

xray/backends/memory.py

+        if variables is None:
+            self._variables = OrderedDict()
+        else:
+            self._variables = variables


could also put these on one line each:

self._variables = OrderedDict() if variables is None else variables self._attributes = OrderedDict() if attributes is None else attributes

shoyer · 2014-10-09T18:45:08Z

should be good to go once you get all the tests passing.

shoyer · 2014-10-11T06:29:23Z

@akleeman I went ahead and pushed fixes for the failing tests. Feel free to merge if when you're ready.

Modular encodings (rebased)

shoyer · 2014-10-11T21:31:20Z

Merged, so I can do some more encodings work on top of this.

Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 3.1.1 to 3.1.4. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v3.1.1...v3.1.4) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Tom Nicholas <[email protected]>

shoyer added topic-backends topic-internals labels Oct 2, 2014

shoyer closed this Oct 5, 2014

akleeman reopened this Oct 8, 2014

shoyer reviewed Oct 8, 2014
View reviewed changes

shoyer mentioned this pull request Oct 8, 2014

Modular encoding #175

Closed

shoyer added this to the 0.3.1 milestone Oct 9, 2014

shoyer reviewed Oct 9, 2014
View reviewed changes

Responded to shoyer's comments

04e0195

shoyer reviewed Oct 9, 2014
View reviewed changes

shoyer added 2 commits October 10, 2014 20:53

abbreviate InMemoryDataStore.__init__

e9d4426

Fix failing tests without deps or not on Python 2.7

ad7df2b

shoyer force-pushed the modular-encoding-v2 branch from b33b747 to ad7df2b Compare October 11, 2014 05:37

shoyer added a commit that referenced this pull request Oct 11, 2014

Merge pull request #245 from xray/modular-encoding-v2

8839723

Modular encodings (rebased)

shoyer merged commit 8839723 into master Oct 11, 2014

shoyer deleted the modular-encoding-v2 branch October 11, 2014 21:30

shoyer restored the modular-encoding-v2 branch October 11, 2014 21:30

shoyer mentioned this pull request Oct 13, 2014

Encoding improvements #251

Merged

shoyer deleted the modular-encoding-v2 branch October 23, 2014 06:27

shoyer mentioned this pull request Jan 29, 2016

coordinate variable not written in netcdf file in some cases #733

Closed

shoyer mentioned this pull request Dec 1, 2017

_FillValue and missing_value not allowed for the same variable #1749

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modular encodings (rebased) #245

Modular encodings (rebased) #245

shoyer commented Oct 2, 2014

shoyer commented Oct 5, 2014

shoyer Oct 8, 2014

shoyer commented Oct 8, 2014

akleeman commented Oct 9, 2014

shoyer commented Oct 9, 2014

shoyer Oct 9, 2014

akleeman Oct 9, 2014

shoyer Oct 9, 2014

shoyer Oct 9, 2014

shoyer commented Oct 9, 2014

shoyer commented Oct 11, 2014

shoyer commented Oct 11, 2014

Modular encodings (rebased) #245

Modular encodings (rebased) #245

Conversation

shoyer commented Oct 2, 2014

shoyer commented Oct 5, 2014

shoyer Oct 8, 2014

Choose a reason for hiding this comment

shoyer commented Oct 8, 2014

akleeman commented Oct 9, 2014

shoyer commented Oct 9, 2014

shoyer Oct 9, 2014

Choose a reason for hiding this comment

akleeman Oct 9, 2014

Choose a reason for hiding this comment

shoyer Oct 9, 2014

Choose a reason for hiding this comment

shoyer Oct 9, 2014

Choose a reason for hiding this comment

shoyer commented Oct 9, 2014

shoyer commented Oct 11, 2014

shoyer commented Oct 11, 2014