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

Issue warnings instead of ModelicaFormatError for missing paths #21

Closed
hyupme opened this issue Jul 25, 2017 · 17 comments
Closed

Issue warnings instead of ModelicaFormatError for missing paths #21

hyupme opened this issue Jul 25, 2017 · 17 comments

Comments

@hyupme
Copy link
Contributor

hyupme commented Jul 25, 2017

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

  • Setting its value to zero
  • Generating a warning message for the missing parameter.
    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.

@tbeu
Copy link
Contributor

tbeu commented Jul 25, 2017

Modelica 4 brought ModelicaWarning.

@tbeu
Copy link
Contributor

tbeu commented Jul 28, 2017

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
Copy link
Contributor

tbeu commented Aug 5, 2017

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.

tbeu added a commit that referenced this issue Aug 8, 2017
* 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).
@hyumo
Copy link

hyumo commented Aug 9, 2017

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
Copy link
Contributor

tbeu commented Aug 9, 2017

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.

tbeu added a commit that referenced this issue Aug 10, 2017
* 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).
@tbeu
Copy link
Contributor

tbeu commented Aug 10, 2017

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

@hyumo
Copy link

hyumo commented Aug 10, 2017

Will do. Thanks @tbeu

I will probably obsolete the username hyupme

@hyumo
Copy link

hyumo commented Aug 15, 2017

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
Copy link
Contributor

tbeu commented Aug 17, 2017

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
Copy link

hyumo commented Aug 17, 2017

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.

  • A tableEnable flag, or a consolidated file reader controlled by an enum parameter. I haven't had a clear picture about this yet, but what I want to achieve is something like if someone has 3 table input options in a model: csv, xml, xlsx the user wants to select between them after the model is compiled. The tableEnable flag should ignore the constructor function if set as false, or a consolidated reader can select a certain constructor by a enum parameter. In this way only 1 constructor function is called, we don't need to place the other 2 as place holders. Similarly, ModelicaStandardTable is using "NoName" to redirect file/table sources.
  • Dynamic 2D table sizes instead of fixed size tables, I guess a getTableSize type function is needed, so that the size of the table does not need to be specified before compilation.
  • Table access functions, e.g. getRow(obj, nRow), getElement(obj, nRow, nCol) etc
  • SDF or HDF5 support? I know there are already some other libraries out there for this, I don't know how their licensing stuff works, but it would be great if ExternData is used as a centralized library for different types of standard formats.

@hyumo
Copy link

hyumo commented Aug 17, 2017

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
Copy link
Contributor

tbeu commented Aug 18, 2017

Thanks for your valuable feedback @hyumo, Some remarks

  • MAT files are pretty popular for parameterization as it covers structures, scalars, arrays and HDF5 (and are well supported by MSL and ExternData).
  • Text files are also popular for the obvious reason of quick and WYSIWYG editing.
  • What do you need dynamic 2D arrays for? I asked the same question at the previous FMI Design Meeting and the only answer I got, it is for look-up tables. Well, for look-up table the MSL has powerful blocks. For all other cases, 2D arrays with know dimensions are sufficient, I believe.
  • Having one meta-block that covers all current file-specific implementations based on the file extension sounds like a useful extension. However, generally you know the file format your data is. And second, the resulting binary would be largeer since it will contain all static libraries.
  • See HDF5 #13 for HDF5.

@hyumo
Copy link

hyumo commented Aug 18, 2017

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

  • I use 2D tables to store polynomial coefficients and exponents, so if I have a dynamic 2D table I can control the number of terms and fitting type by just changing the size of the table. I guess this might be helpful for weight matrix storage for neural nets as well.
  • In terms of the the meta-block, for a developer of Modelica, it is certain that the format of an external file is known, but for downstream users (consumers of compiled Modelica models), instead of giving them 3 binaries which are csv version, xml version and xlxs version, I'd prefer to make 1 binary which is capable of loading 3 types, even though the binary is a bit larger. It is much easier to maintain and avoids redundancy.

tbeu added a commit that referenced this issue Sep 6, 2017
tbeu added a commit that referenced this issue Sep 27, 2017
* 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).
@tbeu
Copy link
Contributor

tbeu commented Sep 27, 2017

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.

tbeu added a commit that referenced this issue Sep 28, 2017
* 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).
tbeu added a commit that referenced this issue Sep 28, 2017
* 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).
tbeu added a commit that referenced this issue Sep 28, 2017
* 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).
@hyumo
Copy link

hyumo commented Sep 28, 2017

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
Copy link
Contributor

tbeu commented Sep 28, 2017

@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 added a commit that referenced this issue Sep 28, 2017
* 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).
tbeu added a commit that referenced this issue Sep 29, 2017
* 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).
tbeu added a commit that referenced this issue Sep 29, 2017
* 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).
tbeu added a commit that referenced this issue Sep 29, 2017
* 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).
tbeu added a commit that referenced this issue Sep 29, 2017
* 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).
tbeu added a commit that referenced this issue Sep 29, 2017
* 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).
tbeu added a commit that referenced this issue Sep 29, 2017
* 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).
@tbeu
Copy link
Contributor

tbeu commented Sep 29, 2017

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

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

No branches or pull requests

3 participants