Skip to content
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

Fix multi-dimensional indexing bug, add tests, and some convenience r… #9

Merged
merged 1 commit into from
Nov 13, 2016

Conversation

tmrn411
Copy link
Contributor

@tmrn411 tmrn411 commented Nov 12, 2016

I've been trying to use jmatio to read multi-dimensional arrays created in matlab into a scala app. It appears there is a bug in the way the indices are computed. In the MLArray constructor dimsFactors is initialized from the highest dimension to the lowest, but Matlab arrays are stored in column major format. So I think dimsFactors should really be initialized starting with dimension 0 and going up. This seems like a fatal bug for anyone trying to transfer multi-dimensional arrays between matlab and jmatio. So I'm surprised it hasn't been hit by anyone else. So maybe I'm just missing something about the library usage. Please review and let me know if you agree.

Simply fixing the way dimFactors is computed would break any s/w that needs to access multi-dimensional files previously created with this library. So I created added dimStrides and a new function getIndexCM (for column major) that can be used to compute the linear index in a Matlab compatible way. I added several convenience accessor functions as well that need to use getIndexCM rather than getIndex.

In addition to the code fixes, I added a matlab sub dir under src/test that contains matlab scripts for generating the .mat files used by the unit tests that I added.

@nedtwigg
Copy link
Member

Fantastic work, thanks very much! I didn't believe that a bug this horrific could possibly have survived undetected for so long, but there's an overload for getIndex(int m, int n) which has the correct column-major behavior. Which leads to this catastrophe:

int arg2 = array.getIndex(3, 2);
int arr = array.getIndex(new int[]{3, 2});

Obviously these should give the same result, but they do not. They do give the same result if the array happens to be square - and most of the test cases use square or cubic arrays, which has helped mask this.

This means that if we just fixed this behavior (no CM methods, just fix the actual methods), then all users of 1D and 2D matrices will be unaffected, as well as users of "square" N-D matrices.

I think this calls for a major-version bump which calls out the error in the previous versions, and making the CM methods be the real methods. I'll merge your change, make these adjustments, and publish as 3.0.0.

I'd like to credit you in the acknowledgements, would you like to be tmrn411 or something else? Any profile you'd like me to link to?

@nedtwigg nedtwigg merged commit 34e707f into diffplug:master Nov 13, 2016
@tmrn411
Copy link
Contributor Author

tmrn411 commented Nov 13, 2016

Thanks. You can list me as Tim Ryan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants