-
Notifications
You must be signed in to change notification settings - Fork 31
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
Issue warnings instead of ModelicaFormatError for missing paths #21
Comments
Modelica 4 brought ModelicaWarning. |
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? |
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. |
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
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 |
Please hang on. c72fc34 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. |
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
Will do. Thanks @tbeu I will probably obsolete the username hyupme |
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 |
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). |
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.
|
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 |
Thanks for your valuable feedback @hyumo, Some remarks
|
Thanks @tbeu , this is quite helpful, I wasn't aware that v7.3 mat was actually HDF5. I will give it a try.
|
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
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. |
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
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. |
@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. |
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
* Report validity by new Boolean output flag ''exist'' (on scalar get functions only) * Use ModelicaMessage (and not ModelicaError) (Can be changed to ModelicaWarning once there is proper tool support).
FYI: I also added the getArraySize2D for XLS/XLSX. |
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 useModelicaFormatMessage
function to generate warnings instead. In order to let the path search continue, we can use a default value0
. 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 toolsI don't know if this is specified in the spec, but the way most tools treat missing parameter values is by
zero
ExternData could follow the same pattern.
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.The text was updated successfully, but these errors were encountered: