Add FMU version to oscillator example #322
Conversation
|
The above described change to the python solver was backrolled. Both solvers now use force-force exchange. Additionally, source code and build instructions for the FMU model were added. |
|
This case also works with Quasi-Newton acceleration for both solvers (Python, FMI and the combination of both), as long as parallel-implicit coupling is used. For serial-implicit coupling and QN acceleration, the simulation fails for both solvers. This seems to be connected to data exchange when the second participant is already converged, but not the first one. |
There was a problem hiding this comment.
Looks good already 👍
I was able to run the case and had a big-picture look at everything. I did not yet look at runner.py in detail since this is probably the wrong place. Or should I already give feedback here? Mid-term goal would be to move runner.py to a separate place, right? Should we merge now already or wait till this moving has been done? In the latter case, the documentation needs updating.
I had a look at the configuration though. We still need some tweaking there (see below)
It seems a bit odd to move the fmu source files into a subfolder cmake. It this an FMU standard way of doing things? More natural names could be Oscillator or fmu. In a cmake subfolder I would expect CMake files (like here: https://github.com/precice/precice/tree/develop/cmake)
Currently, this tutorial runs with preCICE v2. Apparently the oscillator tutorial was not yet updated (correct? @BenjaminRodenberg). Maybe it is indeed simpler to do this later, but let's maybe open an issue on this.
Requires a changelog entry.
oscillator/README.md
Outdated
| @@ -22,9 +22,12 @@ Note that this case applies a Schwarz-type coupling method and not (like most ot | |||
| This tutorial is only available in Python. You need to have preCICE and the Python bindings installed on your system. | |||
There was a problem hiding this comment.
I would simply replace this sentence with another intro statement ... there are two different implementations ...
oscillator/fmi/calculate-error.py
Outdated
There was a problem hiding this comment.
What is this file for? Does it need documentation in README.md? And is it really specific to the fmi solvers or could we generalize it to work for all solvers?
There was a problem hiding this comment.
calculate-error.py calculates the error between the numeric and the analytic solution. When run.sh is used to run the fmi solver, calculate-error.py is called and the error is printed after the simulation.
calculate-error.py is specific to the fmi solver because it has to read both the setting and output file to get the parameters.
The python solver calculates the error during the simulation already and prints it after finishing. The parameters are available within the python file but not as a separate file.
Therefore, I can't bring the calculation of both solvers together in one script.
There was a problem hiding this comment.
👍 Please briefly document in the README
Yes. oscillator.py still uses the v2 API. I quickly created a PR with the necessary changes to update: #332 |
Correct, I'm currently in the process of moving |
There was a problem hiding this comment.
Looks good, a few more minor remarks.
Biggest issue is that now develop of this tutorial was already ported to preCICE v3. I would suggest the following:
- After fixing the remaining issues here, you branch away from here to
create-fmu-oscillator-v2in your fork. - We then first release a v2-compatiable version of the runner v0.1. In the README of this version we link to this v2 branch.
- Both these versions (runner and tutorial) go on DaRUS
- Then (after handing in your thesis), we rebase and update here to v3. And then merge after another review.
There was a problem hiding this comment.
Let's remove such cosmetic changes from this PR. Otherwise, it gets too complicated to overview.
oscillator/fmi/fmu/README.md
Outdated
| cd build | ||
| cmake -DFMI_TYPE=CS -DFMI_VERSION=3 .. | ||
| make | ||
| mv ./Oscillator.fmu ../.. |
There was a problem hiding this comment.
| mv ./Oscillator.fmu ../.. | |
| cp ./Oscillator.fmu ../.. |
A bit more standard IMO.
oscillator/fmi/fmu/LICENSE.txt
Outdated
There was a problem hiding this comment.
I would suggest to do the same trick here as in the runner repo:
- Remove you changes from this file. All your changes are licensed under the main license of the tutorials repo.
- Move this license into a
thirdpartyfolder and explain in situation in thefmu/README.md.
oscillator/README.md
Outdated
| @@ -22,9 +22,12 @@ Note that this case applies a Schwarz-type coupling method and not (like most ot | |||
| This tutorial is only available in Python. You need to have preCICE and the Python bindings installed on your system. | |||
There was a problem hiding this comment.
I would simply replace this sentence with another intro statement ... there are two different implementations ...
|
Before merging, we should also still investigate where the difference in results between FMU and Python participants comes from. |
|
I prepared a version which runs with preCICE v3 on a separate branch |
Could you please open a PR for that one? I guess, we then close this PR, right? |
|
Closing in favor of #465 |
A second version of the oscillator simulation case was implemented using FMU models.
The python version was changed in one major way: The data that is exchanged between the two participants is no longer the force of the middle spring, but the displacement of the respective masses. This was easier to implement with the FMU models. As both versions share the same precice-config XML, this has to be consistent.