modelica-3rdparty / ExternData

:page_facing_up: Modelica library for data I/O of CSV, INI, JSON, MATLAB MAT, SSV, TIR, Excel XLS/XLSX and XML files
https://doi.org/10.3384/ecp21181141
BSD 2-Clause "Simplified" License
74 stars 30 forks source link

Issue warnings instead of ModelicaFormatError for missing paths #21

Closed hyupme closed 7 years ago

hyupme commented 7 years ago

Personally, I actually prefer aborting the simulation if there's any missing path, but there are also some benefits to be discussed if we set a default value and continue the simulation, let's see if there's maybe a better way to handle it.

Instead of generating ModelicaFormatError for missing paths, we can use ModelicaFormatMessage function to generate warnings instead. In order to let the path search continue, we can use a default value 0. I am not quite sure about arrays though, I haven't looked into the C code.

Benefits

Users don't need to run multiple simulations to figure out all of their missing paths.

In the current implementation (i.e. XMLFile), if a certain node is not found, the simulation aborts right away due to ModelicaFormatError. Therefore, it will take a while for an end user to figure out where all his/her missing paths locates.

In Modelica, missing parameter values are default to zero in most tools

I don't know if this is specified in the spec, but the way most tools treat missing parameter values is by

More flexibility in XML file versioning (To be discussed)

In our case, we find that when we tried to test new models with older version XML files, new models simply won't run because of missing paths. This is, of course, an expected behavior, but it also means we need to update these old version XML files to adapt to the new changes even if most of the paths are still valid.

Please let me know if these points are valid. We can probably add a flag into the findValue function to skip errors or something like that.

tbeu commented 7 years ago

Modelica 4 brought ModelicaWarning.

tbeu commented 7 years ago

Unfortunately there is not NaN in Modelica. I actually thought about this problem before, especially for searching colums/rows of Excel files. Currently it is not possible to stop search on an invalid cell. The Modelica environment shall know if an Excel cell or XML node is valid or not. Returning default zero and printing a message is not enough therefore. Any idea, except of introducing another return flag?

tbeu commented 7 years ago

No better idea? Thus, I am going to add a Boolean output flag to scalar functions getReal, getInteger, getBoolean and getString and change the ModelicaError to a ModelicaMessage. The flag returns true if the corresponding path/node/cell exists or false if it does not and a default value is returned.

hyumo commented 7 years ago

Sorry I was not available in the past few days.

I was thinking about adding a global parameter which controls how a user wants to deal with missing key/cells/nodes.

For example, I prefer to issue errors once the model is exported as binaries, it simply prevents file format errors. When debugging and developing, I prefer it just issue warnings so that I can have a list of missing keys/cells/nodes, and default empty parameters to 0 which lines up with what tools are doing now when a parameter has no default values.

I noticed the commit c72fc34, I will give it a try. thank you so much @tbeu

tbeu commented 7 years ago

Please hang on. c72fc3435a375f0da1169510644fa400919c0610 is not yet ready. I forgot some changes ModelicaError -> ModelicaMessage which I will add soon. What I did already is the addition of a new exist flag. This way you can check if a XML node is valid or not.

tbeu commented 7 years ago

@hyumo @hyupme Feel free to test and give feedback.

hyumo commented 7 years ago

Will do. Thanks @tbeu

I will probably obsolete the username hyupme

hyumo commented 7 years ago

Hi @tbeu , I tried branch issue#21-exists, I really liked it.

One issue I found out with 2D xml table reading (I guess it affects 1D table reading as well) is that when the table name is missing, dymosim.exe crashed, so I guess there's something happened in the C code.

I tried the incorrect 2d table names with xls and xlsx reader, they both worked as expected.

I don't have a debugger at hand right now, but the last message it prints out looked something like this image

tbeu commented 7 years ago

Thanks for your feedback. I will fix the access violations in ED_getDoubleArray1DFromXML.

I am currently not that happy with the inconsistency that ED_getDoubleArray2DFromXML, ED_getDoubleArray2DFromXLS(X) print a message on invalid nodes/cells, however ED_getDoubleArray2DFromMAT terminates the simulation on invalid variables (from a MAT file). The reason is that ED_getDoubleArray2DFromMAT directly calls ModelicaIO_readRealMatrix (from the C_Sources Modelica Standard Library, and there ModelicaError is called).

I rather prefer to let all array functions fails in case of invalid input (and consider your enhancement only on the scalar function).

hyumo commented 7 years ago

Yeah, I think for tables it makes sense to fail due to invalid input.

I am also thinking several improvements in the future as well, we can discuss about this in the future, I can also contribute some code as well if this is needed, e.g.

hyumo commented 7 years ago

Hi @tbeu , I am curious if you know how most people are managing their input data? It is such a pain for me right now, we have so many formats (standard and customized) to manage and if, for example, a new parameter is added in the xml file, we have to change the code that generates those files, update database etc.... I am always trying to think of a better solution for this

tbeu commented 7 years ago

Thanks for your valuable feedback @hyumo, Some remarks

hyumo commented 7 years ago

Thanks @tbeu , this is quite helpful, I wasn't aware that v7.3 mat was actually HDF5. I will give it a try.

tbeu commented 7 years ago

As expected, it took me hours resolving the merge conflicts (and fixing the found JSON issues) when merging the issue21-exists branch back to master. I need to test more before I push the binaries and file a new release.

hyumo commented 7 years ago

thanks @tbeu , greatly appreciated. Let me know if I could help anything. It is not a very good timing that the JSON and tir file reader came in during this issue, but contribution is always a good thing.

tbeu commented 7 years ago

@hyumo Thanks for your feedback. Well, it was not good timing keeping this issue open far too long. Anyway, I think I fixed all of the newly introduced JSON problems this morning and am confident to publish a release soon. For your other two comments on dynamic tables and meta-block we would need separate issues then.

tbeu commented 7 years ago

FYI: I also added the getArraySize2D for XLS/XLSX.