search

JuliaIO / MAT.jl

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

fix reading of mxOBJECT_CLASS #57

Closed tanmaykm closed 8 years ago

tanmaykm commented 8 years ago

reading of class name was misplaced, resulting in corruption/errors.

tanmaykm commented 8 years ago

cc @andreasnoack

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 90.485% when pulling 12d3bd1ac181a9a565a62f6046885ce72d964b23 on tanmaykm:fixobjread into 58ed350d4d275eba75b707d6a870826a0fef331c on simonster:master.

timholy commented 8 years ago

Would benefit from a test, if you can create a relevant file.

tanmaykm commented 8 years ago

This file here https://www.dropbox.com/s/6b2hi253otmkonj/3255538_0001.mat?dl=0 displays the issue. I think it is too big to be included for tests. It can be trimmed (to just A that's in it), but I do not have access to matlab. It would be great if someone can trim it or make an equivalent one.

timholy commented 8 years ago

When I try to read it in matlab, I get:

Warning: Class 'Assoc' is an unknown object class or does not have a valid 'loadobj' method.  Object 'A' of this class has been converted to a structure.

So I can't trim it down either, the original author will have to do it.

andreasnoack commented 8 years ago

Thanks for looking into this @tanmaykm. I didn't notice the warning when I loaded it in Matlab. I looked at the header and it seems that the file was generated in Octave. Maybe Octave is doing something wrong here?

This file is quite small and has the same issue so maybe we can use it for the test https://www.dropbox.com/s/xrcl5idzvks1dh3/3700696_0079.mat?dl=0

tanmaykm commented 8 years ago

Thanks! Have added it to the tests.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 90.874% when pulling c6bb8ffe8a2e6bbec353d1e5c6c6224c4e167ce5 on tanmaykm:fixobjread into 58ed350d4d275eba75b707d6a870826a0fef331c on simonster:master.

timholy commented 8 years ago

Maybe Octave are doing something wrong here?

Just to check, was the new file generated in Matlab or Octave?

andreasnoack commented 8 years ago

Both are created in Octave. They give the same warning when loaded in Matlab, the same error in MAT.jl and no error in Octave.

timholy commented 8 years ago

Doesn't that suggest that Octave is the one that's broken? Isn't there some risk that you'll break Matlab by making this change?

andreasnoack commented 8 years ago

@tanmaykm What are your thoughts? Matlab is able to read the file, but with a warning which seems better that erroring.

simonster commented 8 years ago

I checked https://maxwell.ict.griffith.edu.au/spl/matlab-page/matfile_format.pdf and @tanmaykm's correction looks right. The warning in MATLAB doesn't seem surprising since you don't have the .m file that defines the object class.

tanmaykm commented 8 years ago

I had the same reasoning as @simonster and the changes seem correct as per the format spec.

On Sat 21 May, 2016 2:26 am Simon Kornblith, notifications@github.com wrote:

I checked https://maxwell.ict.griffith.edu.au/spl/matlab-page/matfile_format.pdf and @tanmaykm https://github.com/tanmaykm's correction looks right. The warning in MATLAB doesn't seem surprising since you don't have the .m file that defines the object class.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/simonster/MAT.jl/pull/57#issuecomment-220716422

timholy commented 8 years ago

:+1:

simonster commented 8 years ago

Thanks!