Update Array to respect FSStore's key_separator #718
Conversation
Codecov Report
@@ Coverage Diff @@
## master #718 +/- ##
========================================
Coverage 99.93% 99.94%
========================================
Files 26 28 +2
Lines 9945 10328 +383
========================================
+ Hits 9939 10322 +383
Misses 6 6
|
| @@ -1950,7 +1950,11 @@ def _process_for_setitem(self, ckey, chunk_selection, value, fields=None): | |||
| return self._encode_chunk(chunk) | |||
|
|
|||
| def _chunk_key(self, chunk_coords): | |||
| return self._key_prefix + '.'.join(map(str, chunk_coords)) | |||
| if hasattr(self._store, 'key_separator'): | |||
There was a problem hiding this comment.
A heads up that #716 deprecates key_separator in favor store._dimension_separator. Looking at this usage, I imagine wrapping stores like LRUCacheStore and ConsolidatedMetadataStore are going to need to set `_dimension_separator as well.
There was a problem hiding this comment.
Okay, thanks for letting me know. Should we hold off on this PR until #716 is in and then revisit?
There was a problem hiding this comment.
Nah, let's get this out as a point release. Just a warning that this "API" will change. It's all kismet.
There was a problem hiding this comment.
The behavior of _chunk_key should not be changed here. Instead, the behavior of FSStore.getitems should be changed to properly remap the keys. See https://github.com/zarr-developers/zarr-python/pull/709/files#diff-565e487a2f60258b6baa2e4db8ef175cc16b8a949651834bd43d0a9f21e07358R1054
| @@ -1950,7 +1950,11 @@ def _process_for_setitem(self, ckey, chunk_selection, value, fields=None): | |||
| return self._encode_chunk(chunk) | |||
|
|
|||
| def _chunk_key(self, chunk_coords): | |||
| return self._key_prefix + '.'.join(map(str, chunk_coords)) | |||
| if hasattr(self._store, 'key_separator'): | |||
There was a problem hiding this comment.
Nah, let's get this out as a point release. Just a warning that this "API" will change. It's all kismet.
|
This PR needs to be reverted, since the solution in this PR breaks the abstraction boundary between array and chunk store, and the problem this PR solves is addressed in #709 by changes to |
|
see #729 |
* FSStore: aflesh out incomplete test and add another. The first test passes after changes to FSStore. The second test fails. * pep8 fixes * TestNestedFSStore: tweak second assertion of test_numbered_groups * FSStore: change ggetitems to return dict with input keys * TestArrayWithFSStore: add key_separator kwarg FSStore constructor * TestArrayWithFSStore: add key_separator arg to store constructor in create_arrray. * revert changes. the logic I need to test actually lives in test_core, not test_storage. * kill some whitespace * add nested tfsstore tests * FSStore: fsstore.listdir now handles nested keys * FSStore: re-order conditional evaluation in listdir * FSStore: use self.fs.find in listdir * Add tests from #718 * Apply suggestion from @grlee77 * Update PartialRead hexdigest values * More hexdigest updates Co-authored-by: jmoore <josh@glencoesoftware.com>
[Description of PR]
closes #717
I added a couple of FSStore test classes to test_hierarchy.py. The "NestedFSStore" class tests the fix on the specific case raised in #717.
TODO:
* [ ] Add docstrings and API docs for any new/modified user-facing classes and functions* [ ] New/modified features documented in docs/tutorial.rst