Sun 09 Jan 2011 08:24:17 PM UTC, comment #11:
Ok pushed and closed
D.
|
Sun 09 Jan 2011 06:40:42 PM UTC, comment #10:
David, all the examples listed in this report seem to work now, so I think you should go ahead and apply the patch and close this rerport.
If someone notices that there is still some problem, then please open a new bug report.
|
Thu 06 Jan 2011 10:18:56 PM UTC, comment #9:
I'm starting to remember why I hate the matlab file format....
The new issue has to do with the way matlab treats small (tag,value) pairs. That is a matlab data element is made up of a number of pairs (tag,value) where the tag contains the type of the value and its size in bytes. Typically the tag is 8 bytes with 4 bytes of the type followed by 4 bytes for the length of value.. and here's the catch. If the size saved in the tag is 4bytes of less, then the type is coded in 2 bytes, the length in two bytes and the data in four bytes!
As the length of a single element in the double type is 8 bytes they were never saved with short tags.. Single elements of the single type however are. So when calculating the length of the data to be saved we now need to take into account short tags for the single precision type.. My copy-paste to create the single-precision load/save code didn't take this into account.
I've attached a new changeset and hopefully this time there are no further issues. The new patch also avoids some copying when calculating the element length by casting the arrays as const.
Given the past history of this bug I won't commit this and close this bug till I have a report that there aren't any other issues.
D.
(file #22365)
|
Thu 06 Jan 2011 04:54:27 PM UTC, comment #8:
Unless I made a mistake when applying the patch, it is still not entirely fixed.
This now works:
a=struct('field1',single(0:1),'field2',single(0));
save a.mat a -v6; load a
But this doesn't work any more:
a=struct('field1',single(0),'field2',single(0));
save a.mat a -v6; load a
Thanks.
|
Wed 05 Jan 2011 10:26:09 PM UTC, comment #7:
Ok, I see the problem.. There was no version of get_save_type function in ls-util.cc for float values and the data was saved as LS_DOUBLE rather than LS_FLOAT as it should have been. Adding a version of this function
extern save_type
get_save_type (float max_val, float min_val);
that returns LS_FLOAT fixes this problem for me. I should probably also replace the code
(nel + 1) & ~0x1LL
with something generic that handles both 32 and 64-bit versions of octave_idx_type cleanly as well and propose replacing this as well. I believe the changeset attached fully fixes this issue, but I prefer feedback before pushing it and closing this bug
D.
(file #22361)
|
Wed 05 Jan 2011 01:07:59 PM UTC, comment #6:
Thanks for this Søren.
Here is another example that crashes and does not need to have MATLAB at hand:
clear a;
a.field1 = single(1:2);
a.field2 = single(0);
save a.mat a -v6;
load a.mat
Thanks.
|
Wed 05 Jan 2011 09:07:52 AM UTC, comment #5:
I have done the following in Matlab 2010a:
>> a = single(pi);
>> b = single(exp(1));
>> save -v6 ab_ml2010a.mat a b
>> clear a
>> a.field1 = single(pi);
>> a.field2 = single(exp(1));
>> save -v6 a2_ml2010a.mat a
The resulting files are attached.
(file #22357, file #22358)
|
Tue 04 Jan 2011 08:58:13 PM UTC, comment #4:
As I said in the bug report and #30800, I no longer have access to a copy of matlab and so can't easily debug this.. My only solution would be to get you to upload the file saved by matlab and compare it against the same octave version byte by byte. Though I'd prefer a simpler file with no structures and one or two variables. Octave doesn't reduce floats to integers when saving them like matlab does (ie. single(1) will be save as a uint8 by matlab to save space). So does doing something like
a = single(pi);
b = single(exp(1));
save -v6 ab.mat a b
fail to create a file loadable by matlab? If it does could you run the same code in matlab and upload the file ab.mat? If not could you try
clear a
a.field1 = single(pi);
a.field2 = single(exp(1));
save -v6 a.mat a
D.
|
Tue 04 Jan 2011 01:39:36 PM UTC, comment #3:
Hi,
It's definitely better but there is still something wrong when saving the file as MATLAB doesn't load it properly.
Try with the following:
clear a;a.field1=single(1);a.field2=single(2);save a.mat a -v6;load a.mat
If you load the the Octave-created 'a.mat' in MATLAB, you get this:
a =
field1: []
field2: []
while the other way round works ('a.mat' created by MATLAB and successfully read by Octave).
Many thanks.
|
Tue 28 Dec 2010 07:46:11 PM UTC, comment #2:
There wasn't any saving of the single precision type in the matlab format till it was added after the bug report #30800 (see https://savannah.gnu.org/bugs/?30800). I made a slight error in this previous fix that prevented single precision arrays with an odd number of elements being saved. The size of the data to save needs to be an integer number of 8 bytes long with 4 bytes of padding if needed (and it was), but the length of the data element saved in the header of the element didn't take this into account.
The changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/a4822f3d1032
addresses this. As this fixes this bug report for me I'm closing it.
D.
PS. I added the reporter of bug #30800 in copy to this message so that he is aware of the issue.
|
Fri 24 Dec 2010 03:46:21 PM UTC, comment #1:
Confirmed on a recent tip (11412:a8a9f062d0ef) on 12/24/2010.
|
Tue 21 Dec 2010 03:54:32 PM UTC, original submission:
In Octave devel (3.3.54+), the following crashes:
returns:
while it works fine with current stable version (3.2.4).
It seems to be due to a bug with the handling of single precision variables.
|