-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid error with MATLAB classes that we can't handle #129
Conversation
Thanks! Yes, I think it would be better to |
Right now you get an error, as in #127. What do you suggest we do with the problematic variable? With this PR, the variable's contents get replaced with a string which I thought was useful, because then you can see that a variable was supposed to be there, rather than silently ignoring it. |
Maybe return |
Ok, so this now works, with a caveat that I hadn't noticed before. If using When using
|
I am locally getting HDF5 read errors ( I can see see that the same thing is happening on master. |
Ok, it seems that the error does not happen right after recompiling, but if I exit julia after running the tests, and then try running the tests again in a fresh session there it crashes. Then it keeps crashing, until I trigger a recompilation at which points it works once and the cycle begins again... |
The error does seem to be amplified by my changes, but I can't tell why... The code path is clearly not being called or we would see the warning happening. |
Where does
Oh dear, I hate these kind of things. I saw it on Travis for Julia 0.7, but you're getting it on a more recent version? Might be an HDF5.jl bug, I would start by looking into that line |
I've beeen trying to rebase this but the Github action tagbot is preventing me from pushing changes... Do you know how this is supposed to work? |
Sigh, I had to add everything as SSH connections for it to work. Let's see what CI says now. |
Ok, it seems that the only error is the same one that happens on master. Locally tests pass for me. |
Ok, so I went down the rabbit hole with this, and ended up seeing PR #23. The I have excluded the entry from the I have also adapted the above PR to allow reading |
@timholy I believe this is ready. The 1 CI failure has been seen on master, e.g. |
Small bump here: I think this is ready |
@timholy or @simonster Would you mind merging this PR? I have been using it for the past months without issues. It would help with #128 and #133. |
@jebej sorry this got missed, thanks! |
This is similar to JuliaIO#129 which returns `missing` for unhandled data types in HDF5 Datasets; function handles are stored as HDF5 groups, so a different explicit check is required to skip over them.
This is similar to #129 which returns `missing` for unhandled data types in HDF5 Datasets; function handles are stored as HDF5 groups, so a different explicit check is required to skip over them.
This method simply avoids classes that cannot currently be handled without crashing the read, and at least allows looking at the rest of the file.
Maybe logging a warning would be better than just replacing the object with a string. Happy to change this to whatever is preferred.