-
Notifications
You must be signed in to change notification settings - Fork 32
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
Evaluation in Wolfram System Modeler #52
Comments
@beutlich I tested it with System Modeler on Linux-64. These seem to work fine:
For many examples I get this from the simulation:
These are the examples with said error message:
For
For XMLTestInnerOuter I get a build failure. I suspect this may be a System Modeler bug:
For |
(I'm guessing the library is still illegal Modelica per modelica/ModelicaSpecification#2399 , so I'm disappointed we don't just reject all examples during build.) |
Thanks for evaluating.
Did you also check the results?
These models call
I belive it hides the same issue of "empty" values.
That model actually is a workaround only.
Hopefully it is valid Modelica though there are spec. issues on external objects that should have been clarified years ago. |
I plotted them, but just to check they contained data. I didn't verify against anything.
https://specification.modelica.org/master/class-predefined-types-and-declarations.html#S7.I1.i1.p1 says:
which seems to exclude |
If it's just the records - they got introduced by #18. Here's the same lib with that change reverted back to model: https://github.com/modelica-3rdparty/ExternData/tree/no-records Does it work in WSM? |
After further reading of the specification, it is my interpretation that WSM does the right thing for
The functions that This is the relevant part of the specification:
That part is fine.
This example is trying to reference parts from the |
Thanks for the feedback. Regarding the Otherwise, I believe I can safely remove this example and the corresponding types |
Thanks for spending time on this. The I don't know what the spec says (and our local expert is unavailable at the moment), but to me it seems a little bit sketchy that the new partial functions don't have the inputs they are called with in |
OK, so it seems WSM is another tool to be added as supported (in the Readme). I ported the fix-inner-outer changes to master and rebased the no-records branch accordingly. My other question is, if it is worth to keep the no-records branch alive, maintain (and release) it as alternative version. |
I would be curious to see the warning from Dymola mentioned in #17, to see if there is something else illegal with the non-record variant WSM should have caught. |
You might have noticed that I needed to remove the The other advantage of records over models is that it can be declared as |
Thank you, I hadn't noticed that. It looks like only
and called like this?
Unfortunately I'm on my mac today, so I can't test it past validation in WSM (since ExternData doesn't have macOS binaries). |
Yes, that is possible and certainly a reasonable change. Fixed by 713ede3 in master. The branch no-records is rebased accordingly. Regarding the missing macOS binaries: Would be some work to add the hdf5 lib specifically. |
Then the remaining question is whether being able to declare the |
I tested the no-records branch on Windows using System Modeler, and none of the examples build. I get the following error: `sme.14.3.0_1738155665_1533310374_ext1-get.obj : error LNK2005: timezone already defined in sme.14.3.0_1738155665_1533310374_ext0-get.obj sme.14.3.0_1738155665_1533310374_ext1-get.obj : error LNK2005: _arrayTimezone_initialize already defined in sme.14.3.0_1738155665_1533310374_ext0-get.obj sme.14.3.0_1738155665_1533310374_ext1-get.obj : error LNK2005: Timezone_initialize_wrapper already defined in sme.14.3.0_1738155665_1533310374_ext0-get.obj sme.14.3.0_1738155665_1533310374_ext1-get.obj : error LNK2005: __iob_func already defined in sme.14.3.0_1738155665_1533310374_ext0-get.obj` |
As @ankitnaik177 mentioned, we do get linking errors on Windows. I think this is because we build each external function separately, and then link everything together. Maybe something like |
Following the TDD approach and easiest for me is to have the linker error reproducable in the CI job test_cmake_windows-msvc. Feel free to file a PR. Otherwise it is too much guessing for me. |
Since the library has several users on Dymola I need to consider compatibility and must not break existing models if #18 gets reverted. Hence I hope for a clarification in Modelica Language first and rather would postpone the decision until modelica/ModelicaSpecification#2399 is resolved. |
I could easily reproduce in the unit test setup. I added ee72766 to the no-record branch that demonstrates both the issue and its mitigations. Using the |
I'm hesitant to add that flag to System Modeler. The specification puts this requirement squarely on the library side:
maltelenz@90168c1 seems to avoid two more cases, but I wasn't able to get rid of this final one:
|
Did you try to set the linker option
SimulationX has set
Ah, never noticed this before. Thanks for the update. And I now see where the new paragraphs originate from: modelica/ModelicaSpecification@7fe3b48 for modelica/ModelicaSpecification#3452 / modelica/ModelicaSpecification#3451. In that sense, ExternData is still on MLS 3.6 (being latest officially MA approved MLS): https://specification.modelica.org/maint/3.6/functions.html#annotations-for-external-libraries-and-include-files and as such somehow relys on features that might get deprecated in a future MLS version. So it's kind of a non-issue, right? Modelica-wise, how can a library developer guarantee that a certain "Include" content is built exactly once - under all circumstances - as compilation unit? Inner/outer following the |
The entire MSVC compatibility hack can be avoided if all libraries are compiled by the same compiler as the final binary. That's the proper way to go. |
No, I didn't try that yet. I am still hoping to avoid it.
Technically, this is correct. In practice, WSM already builds external functions in separate units, since putting everything in the same translation unit/file has other, harder to fix, problems.
This is not what the change to the specification asks for. What the specification asks for is that one/each external function can be built standalone using its include annotation, in a way that many such builds (of different functions) can later be linked together into one binary. This is to allow tools to build each external function separately, so that different external functions do not interfere with each other (polluting namespace using include, defines, ...). |
@otronarp @maltelenz I wonder how the library v3.1.0 evaluates in WSM. SinceI do not have access to WSM it would be helpful if you could run the examples and give feedback here. Thanks.
The text was updated successfully, but these errors were encountered: