Allow VLenUTF8 to encode a read-only source #515
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #515 +/- ##
=======================================
Coverage 99.91% 99.91%
=======================================
Files 59 59
Lines 2336 2339 +3
=======================================
+ Hits 2334 2337 +3
Misses 2 2
|
There was a problem hiding this comment.
This seems like a simple improvement with no backwards incompatibilities.
I'm sorry @ivirshup that no one reviewed this sooner. It has been hard to find maintainers for numcodecs.
Changes documented in docs/release.rst
Would you care to document this change? If not we can just merge as is, as this is pretty minor.
| @@ -74,7 +75,7 @@ class VLenUTF8(Codec): | |||
| def encode(self, buf): | |||
| cdef: | |||
| Py_ssize_t i, l, n_items, data_length, total_length | |||
| object[:] input_values | |||
| ndarray[object, ndim=1] input_values | |||
There was a problem hiding this comment.
Isaac, could you please share a bit more about why this is needed?
There was a problem hiding this comment.
I don't remember that much of the specifics, but it has to do with which types Cython is able to work with a read-only buffer. I remember running into this a bit with pandas too when I tried to make the index arrays actually read-only.
Is that what you were wondering about?
There was a problem hiding this comment.
More info here: #514 (comment)
* fix? * test * format * add release notes --------- Co-authored-by: Ryan Abernathey <ryan.abernathey@gmail.com>
Allow read-only arrays to be encoded with VLenUTF8
Arguably this should be tested and potentially fixed for all codecs.
Question: Is this implementation fine? I've done very little Cython, so I have no idea if there are any downsides to this.
TODO: