Closed rcannood closed 2 months ago
I created a PR (#209) which contains a unit test for this edge case.
I think the issue occurs whenever there's a function from the HDF5 library that expects a buffer.
if(XLENGTH(R_buf) == 0) {
buf = NULL;
}
else {
buf = ...
}
// ...
... = H5SomeFunction(..., buf, ...);
It appears this buffer shouldn't be NULL (otherwise the error H5SomeFunction(): buf parameter can't be NULL
is thrown).
Question is, with what should each of these buf = NULL
be replaced? I've had some success by replacing buf = NULL;
with buf = R_alloc(1, 1); // allocate 1 byte
, but that causes other problems down the road.
I managed to solve the issue by replacing the XLENGTH(.) == 0
condition with TYPEOF(.) == NILSXP
:
const void* buf;
if(TYPEOF(R_buf) == NILSXP) {
buf = NULL;
}
else {
buf = (void *) VOIDPTR(R_buf);
}
I created PR #211 which allows the unit tests I added in #209 to pass. It should be noted that I only performed the substitution only where needed for allowing the tests in #209 to pass.
sorry for the late reply. Will try to look at the fix over the Christmas break and thanks for the PR.
No worries and thanks! Is there anything I can help out with? Would it help to schedule a call?
just a quick update. I had an initial look at this, but I need to dig deeper. A purely technical issue is that the places where you made the fix all need to be moved. The reason is that this code is auto-generated, so the way the code is generated has to be adapted upstream. As that code is somewhat ... ugly, I need to do that adaptation on my end.
Stay tuned
Thanks for looking into this!
this code is auto-generated
It's good to hear that the code is auto-generated. I couldn't imagine maintaining this code base across different versions of HDF5 library.
Out of curiosity, is the script that generates the code in a public repository?
Fixed in #214.
You can look for the for the code generates the wrapper files under /inst/CWrappers... but beware, it is not pretty.
Aha, that's what the patches are for! Makes sense :)
Thanks for fixing the issue!
@rcannood : I had to revert the changes for now and reopen the issue. The hdf5r.Extra package is causing a reverse dependency error.
https://github.com/ycli1995/hdf5r.Extra/issues/1
The code for the fix is located in the branch
https://github.com/hhoeflin/hdf5r/tree/bug/fix_empty_attrs_again
It seems @ycli1995 fixed https://github.com/ycli1995/hdf5r.Extra/issues/1 :relaxed:
Could we get this fix merged and released? :bow:
Hi @hhoeflin!
I ran a revdepcheck on this branch:
> revdepcheck::revdep_check(timeout = as.difftime(600, units = "mins"), num_workers = 8)
── CHECK ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 13 packages ──
✔ readNSx 0.0.2 ── E: 0 | W: 0 | N: 0
✔ hdf5r.Extra 0.0.5 ── E: 0 | W: 0 | N: 0
✔ rblt 0.2.4.6 ── E: 0 | W: 0 | N: 0
✔ dynutils 1.0.11 ── E: 0 | W: 0 | N: 0
I didn't let it finish completely, but I can already confirm that this PR does not introduce new errors to hdf5r.Extra
anymore.
Would you be able to merge the PR? :bow:
Will merge soon. There is yet some more compiler warnings to fix but this should be merged and out with the next release
On Sat, Feb 10, 2024, 09:38 Robrecht Cannoodt @.***> wrote:
Hi @hhoeflin https://github.com/hhoeflin!
I ran a revdepcheck on this branch:
revdepcheck::revdep_check(timeout = as.difftime(600, units = "mins"), num_workers = 8) ── CHECK ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 13 packages ── ✔ readNSx 0.0.2 ── E: 0 | W: 0 | N: 0 ✔ hdf5r.Extra 0.0.5 ── E: 0 | W: 0 | N: 0 ✔ rblt 0.2.4.6 ── E: 0 | W: 0 | N: 0 ✔ dynutils 1.0.11 ── E: 0 | W: 0 | N: 0
I didn't let it finish completely, but I can already confirm that this PR does not introduce new errors to hdf5r.Extra anymore.
Would you be able to merge the PR? 🙇
— Reply to this email directly, view it on GitHub https://github.com/hhoeflin/hdf5r/issues/208#issuecomment-1936934354, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASGMYUIQXS2M7VVFPEJLO3YS4WYZAVCNFSM6AAAAABALGC7Z6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZWHEZTIMZVGQ . You are receiving this because you were mentioned.Message ID: @.***>
Awesome, thanks!
Hi @hhoeflin !
I see that branch bug/fix_empty_attrs_again
has not yet been merged.
Might I inquire about the status of this fix?
The described issue has been a blocking issue for months on end now, and it's saddening to see that a fix for it exists but is simply not being merged / released.
Happy to help out with whatever you would need help with!
Kind regards, Robrecht
Hey @hhoeflin !
Thanks for releasing hdf5r 1.3.11! :bow: :bow: :bow:
I can confirm this has fixed my issue.
Hi @hhoeflin ! Thanks again for creating and maintaining this incredibly useful package.
I'm having a few issues reading and writing attributes with arrays of length zero. Unfortunately this is quite blocking for a project I'm working on.
Here is a zip file of the h5ad file used in the examples below: example.zip
This is similar though I think unrelated to #118.
Any help in this matter would be much appreciated! I'll look through the hdf5r code to see if there is anything I can do to solve the problem.
Reading an attribute containing an empty array
When I try to read an attribute containing an empty array, I get an error message:
In comparison, this does work in
h5py
:Writing an attribute containing an empty array
I also can't write empty arrays as attributes:
In Python, this does work: