-
Notifications
You must be signed in to change notification settings - Fork 78
Edit: Fix loading datetime with complex valued data in MAT-files #216
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
base: master
Are you sure you want to change the base?
Conversation
|
No unit tests needed you think? |
|
Not sure. Do we just keep adding MAT-files for different MATLAB versions? Will be good to make sure we don't break anything backwards, but I don't have access to the latest MATLAB version to generate test cases. Any case, don't think this is ready to merge yet as the intended behaviour needs to be better understood for both issues addressed in this PR. |
|
You know what, I'll install 2024b for this reason alone :) Will probably take a few hours. I do agree we should be careful with polluting the package with test files. But perhaps this is important enough to add one small 2024 test file? |
|
I've created the request R2024b .mat file for testing, let me know if you need more. I'm also informed that we could potentially get free github cloud licenses to run multiple MATLAB versions in this open source project, see MATLAB actions / setup MATLAB. For example to automatically generate the test files on the latest MATLAB version. If we keep running into similar issues that might be worth the effort. |
|
That's very interesting, thanks! Thanks for the test file as well, I'll study it later. I've been looking at some other MAT-files as well, and so far I've figured out the following: The FileWrapper object contains a version indicator somewhere. However, this only seems to be a major version. I came across a few examples where the FileWrapper metadata format had slightly changed within the same major version. As far as I know, there isn't any indicator of a minor version anywhere. I believe we've covered both major and minor versions up to 4.x. There's a few metadata regions whose purpose we don't know about yet, but which MATLAB has reserved for future updates. So we won't need to worry about any significant changes to the MCOS codebase, Mathworks seems to have planned this out for a long time now. We could add some sanity checks to trigger warnings when the reserved regions contain data, this would also help us keep up to date with MATLAB updates. |
|
Excellent! Let me know whenever you are ready to merge this PR |
|
Turns out it wasn't actually an issue with the new MAT-file, but a tiny bug that passed through on I don't think we need a separate test for this. It would be better to hardcode a test using a dummy However, I think we should split the datetime fix in |
|
Sure, go ahead, and feel free to open or update issues if you fear you might forget your ideas. I assume you'll remove the datetime fix from this PR then? |
9510339 to
486bdab
Compare
|
I changed this to the datetime PR. Edited title to reflect it. Summary of changes:
|
This is a small fix towards #212 and #215 (builds on #213 ) where it looks like MATLAB has made a few subtle changes in newer versions:
MATLAB datetime appears to store some sort of time precision in a complex number. I'm not sure what it is exactly, and until it can be figured out this a temporary fix to prevent errors due to float remainders when converting to Julia DateTime.
Newer MATLAB versions seem to maintain a separate object ID counter for some built-in types like
stringand maybe eventimetablewhich are serialized using asaveobjmethod. This means that these objects no more contain adep_idwhich was used to deserialize dynamic properties. The fix takes care of this case. This update shouldn't be a problem for writing as MATLAB should be maintaining backward compatibility.