JuliaIO / MAT.jl

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

Read Modelica MATv4 files #181

Open bc0n opened 1 year ago

bc0n commented 1 year ago

This adds support for reading v4 MAT files written by OpenModelica, according to the format detailed here. This Modelica format is not readable by either #132 or #164.

At this point I'm not exporting any of the operations into MAT.jl. As in test/runtests_modelica.jl they can be accessed directly by MAT.MAT_v4_Modelica.readAclass("matfile.mat"). The function design is very literal right now and will be condensed into all-in-one accessors at some point.

Given the size of MAT files, read operations are atomic. This differs from the MAT.jl interface, as the file pointer is never given to the user. And right now, there is now ability to write files, mostly because I have no workflow requirement for this. Also my development only included files authored by OpenModelica; Dymola appears to write a slightly different format.

Stepping back, I'd appreciate comments on the design and whether and how the operations for reading OpenModelica MATv4 files can be added to MAT.jl. I was not able to use MAT.jl's basic accessors to read these files because of the explicit v4 test.

sjkelly commented 1 year ago

We now have the version kwarg to the read/write functions. I am not sure of the Matlab v4 and OpenModelica v4 distinction, so if there needs to be a slight different handling, we can add it as a special case.

qnikil7 commented 1 year ago

OpenModelica v4 and Dymola v4 use Matlab v4 format, but have the variables and values stored in a particular format as mentioned by @bc0n above.

DyMat or buildingspy can read these in python.

The above work by @bc0n, reads OpenModelica v4 files, and with slight fixes reads Dymola v4 format with this.

My only concern is whether there would be some repetition in some code, as Modelica .mat is also a matalb v4 files.

Ideally (according to me) the function reading the Modelica .mat file should be using the matlab v4 read function, to return variables or values.

Practically I would love to have this functionality as it is, and and issue could be create to potentially use matread/matopen functions in the future. If this is okay, this PR could be merged as is or we can add Dymola .mat files support as well with sight fixes, as this PR.

sjkelly commented 1 year ago

If we can autovdetect the Matlabv4 and the Modelicav4, I think that would be great. I think this PR certainly needs two things in the interim:

bc0n commented 1 year ago

Thanks @qnikil7 et all, can you also simulate the FallingBodyBox model under Dymola2021 so that we have some coverage into the Modelica Standard Library?

bc0n commented 1 year ago

@sjkelly @qnikil7 I recall trying to use the deprecated v4 reader on Modelica files, but something about how it read the file and the assumed data type persuaded me to roll my own. Kwargs are easy but there may be a better MAT.jl-level design decision to make that more flexibly reads differently-formatted files, something involving schemas?

qnikil7 commented 1 year ago

@bc0n

Created a PR: added FallingBodyBox model under Dymola2021, Removed Pkg dependency.

Also couple of thoughts

bc0n commented 1 year ago

This is ready @sjkelly, the CI fails are in HDF5.jl and _memcpy @latest.

And from before, how or whether this integrates with the open-file approach of the other functions is open to discussion. I added isMatV4Modelica() to support an integration into read(), but again the file sizes encourage retrieving specific variables only when needed.

sjkelly commented 1 year ago

Thanks! Can you rebase this on master? The diff is difficult to read. If you need help rebasing, I am happy to do it for you.

bc0n commented 1 year ago

@sjkelly the rebase was a bit rough but is much clearer now, let me know if you'd like me to open a new pr in place of this one.

bc0n commented 10 months ago

Looks like the _mecpy error has gone away; again let me know if there's anything I can do to get this to merge. Or if it's not a fit I'll put it elsewhere. Thanks

ViralBShah commented 5 months ago

Given the size of this commit, wouldn't it be better to have a separate package?