JuliaIO / MAT.jl

Julia module for reading MATLAB files
MIT License
278 stars 71 forks source link

Avoid error with MATLAB classes that we can't handle #129

Closed jebej closed 3 years ago

jebej commented 4 years ago

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.

timholy commented 4 years ago

Thanks! Yes, I think it would be better to @warn or @error. Can you paste what happens now? And ideally, add a test for this new functionality? You'd need to contribute a small file that has an unreadable data type.

jebej commented 4 years ago

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.

timholy commented 4 years ago

Maybe return missing? Along with the warning that gets printed to the REPL?

jebej commented 4 years ago

Ok, so this now works, with a caveat that I hadn't noticed before.

If using matread, or read without specifying a variable, the read will fail. This seems to be due to a weird key in the names of the file returned by matopen. See below.

When using read with the variable, things work out, and this is how I am running the test currently (using a closure with matopen).

julia> mfile = matopen(path)
MAT.MAT_HDF5.MatlabHDF5File(HDF5 data file: C:\Users\Jeremy\.julia\dev\MAT\test\v7.3\struct_table_datetime.mat, true, false, 36, false)

julia> read(mfile)
HDF5-DIAG: Error detected in HDF5 (1.10.5) thread 0:
  #000: E:/mingwbuild/mingw-w64-hdf5/src/hdf5-1.10.5/src/H5A.c line 425 in H5Aopen(): unable to load attribute info from object header for attribute: 'MATLAB_class'
    major: Attribute
    minor: Can't open object
  #001: E:/mingwbuild/mingw-w64-hdf5/src/hdf5-1.10.5/src/H5Aint.c line 433 in H5A__open(): unable to load attribute info from object header for attribute: 'MATLAB_class'
    major: Attribute
    minor: Can't open object
  #002: E:/mingwbuild/mingw-w64-hdf5/src/hdf5-1.10.5/src/H5Oattribute.c line 515 in H5O__attr_open_by_name(): can't locate attribute: 'MATLAB_class'
    major: Attribute
    minor: Object not found
ERROR: Error opening attribute /#subsystem#/MATLAB_class
Stacktrace:
 [1] h5a_open at C:\Users\Jeremy\.julia\packages\HDF5\Zh9on\src\HDF5.jl:2279 [inlined]
 [2] a_open(::HDF5.HDF5Group, ::String) at C:\Users\Jeremy\.julia\packages\HDF5\Zh9on\src\HDF5.jl:852
 [3] a_read(::HDF5.HDF5Group, ::String) at C:\Users\Jeremy\.julia\packages\HDF5\Zh9on\src\HDF5.jl:1269
 [4] m_read(::HDF5.HDF5Group) at C:\Users\Jeremy\.julia\dev\MAT\src\MAT_HDF5.jl:201
 [5] read(::MAT.MAT_HDF5.MatlabHDF5File, ::String) at C:\Users\Jeremy\.julia\dev\MAT\src\MAT_HDF5.jl:264
 [6] read(::MAT.MAT_HDF5.MatlabHDF5File) at C:\Users\Jeremy\.julia\packages\HDF5\Zh9on\src\datafile.jl:45
 [7] top-level scope at none:0

julia> read(mfile,"s")
┌ Warning: MATLAB datetime values are currently not supported
└ @ MAT.MAT_HDF5 C:\Users\Jeremy\.julia\dev\MAT\src\MAT_HDF5.jl:173
┌ Warning: MATLAB table values are currently not supported
└ @ MAT.MAT_HDF5 C:\Users\Jeremy\.julia\dev\MAT\src\MAT_HDF5.jl:173
Dict{String,Any} with 2 entries:
  "testDatetime" => nothing
  "testTable"    => nothing

julia> names(mfile)
2-element Array{String,1}:
 "#subsystem#"
 "s"
jebej commented 4 years ago

I am locally getting HDF5 read errors (Error reading dataset /imaginary) with check("complex.mat", result), though not consistently.

I can see see that the same thing is happening on master.

jebej commented 4 years ago

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...

jebej commented 4 years ago

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.

timholy commented 4 years ago

If using matread, or read without specifying a variable, the read will fail.

Where does #subsystem# come from? Is it something you added to the file, or did Matlab add that itself? If it's a reproducible name one could systematically exclude it.

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.

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 h5d_read(::Int64, ::Int64, ::Int64, ::Int64, ::Int64, ::Array{Complex{Float64},1}) at /home/travis/.julia/packages/HDF5/Zh9on/src/HDF5.jl:2279

jebej commented 4 years ago

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?

jebej commented 4 years ago

Sigh, I had to add everything as SSH connections for it to work. Let's see what CI says now.

jebej commented 4 years ago

Ok, it seems that the only error is the same one that happens on master. Locally tests pass for me.

jebej commented 4 years ago

Ok, so I went down the rabbit hole with this, and ended up seeing PR #23. The #subsystem# entry is described there.

I have excluded the entry from the names function, meaning everything works now.

I have also adapted the above PR to allow reading datetime values, which I will submit once this is in.

jebej commented 4 years ago

@timholy I believe this is ready.

The 1 CI failure has been seen on master, e.g. https://travis-ci.org/github/JuliaIO/MAT.jl/jobs/647798944 https://travis-ci.org/github/JuliaIO/MAT.jl/jobs/619762537 https://travis-ci.org/github/JuliaIO/MAT.jl/jobs/619738192

jebej commented 4 years ago

Small bump here: I think this is ready

jebej commented 4 years ago

@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.

musm commented 3 years ago

@jebej sorry this got missed, thanks!

jebej commented 3 years ago

Thanks! ~Presumably #133 & #128 can be closed.~

Edit: nevermind