Fix: Attributes look like h5py attributes when using the pyfive backend. #6
Conversation
| # method, but we only have to deal with | ||
| # the case of a numpy array of strings. | ||
| try: | ||
| if output.dtype == object: |
There was a problem hiding this comment.
Does this work ok with attributes? Is output the actual array at this point? In which case I imagine it would work fine. Coz this is all in attributes, not data, right?
There was a problem hiding this comment.
Actually, I wonder whether implementing a check_string_dtype method might solve my other problems with dtypes?
There was a problem hiding this comment.
Yes - this is just attributes. No data arrays here!
Actually, I wonder whether implementing a check_string_dtype method might solve my other problems with dtypes?
I think implementing would indeed work. Looks straight forward: https://github.com/h5py/h5py/blob/master/h5py/h5t.pyx#L1893-L1913.
There was a problem hiding this comment.
Do you fancy doing that in pyfive? It would be consistent with the other stuff you are cleaning up. Then this code can be more consistent h5py and we have some more support for this stuff there.
There was a problem hiding this comment.
Bit of a can of worms.
-
The h5py
check_string_dtypeexpectsh5py.h5a.AttrIDobjects, whereas pyfive gives us numpy or str objects. I could delve into the h5py object to see what's in it, but I do know that it relies in thenp.dtype.metadatamappingproxy, which is something defined elsewhere in h5py (not standard numpy), and (quote from docs) is "long undocumented and is not well supported. Some aspects of metadata propagation are expected to change in the future." -
Then there's python2 code in the function:
if vlen_kind is unicode:(https://github.com/h5py/h5py/blob/master/h5py/h5t.pyx#L1905). I guess this works becausecythonstill understands it, but our own python3 would not. Isunicodein python2 the same asstrin python3. Probably? Am I reading this right?
Anyway, there's enough here to make me want to not implement our own check_string_dtype at this time.
h5netcdf/tests/test.nc
Outdated
There was a problem hiding this comment.
There are no tests in the pull request, so what is this doing here?
There was a problem hiding this comment.
Not sure. I didn't willfully re-make the file! I did run the unit tests. Do they recreate test.nc, and with some bit of difference due to some different library version or other that I was using?
We should revert them, I guess
h5pyattributes when using thepyfivebackend. #5CHANGELOG.rst