Deltares / rtc-tools

The Deltares toolbox for control and optimization of environmental systems.
GNU Lesser General Public License v3.0
0 stars 2 forks source link

Draft: Add an example with a customized variable update #1634

Closed SGeeversAtVortech closed 1 month ago

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Sep 19, 2023, 15:38

Add an example that illustrates how to implement an equation outside of the Modelica file by defining a customized variable update in the python code.

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Sep 19, 2023, 15:38

requested review from @k-horvath-deltares

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Sep 19, 2023, 15:45

added 2 commits

Compare with previous version

SGeeversAtVortech commented 1 month ago

In GitLab by @vreeken on Sep 21, 2023, 22:54

Commented on doc/examples/simulation/customized_update.rst line 73

I find this to be a very import caveat. Saying

The equation for x is given in the Modelica file, while the equation for y is implemented in the python code.

at the start feels somewhat misleading, as the two variables/equations for x and y are not at all the same. One is solved for implicitly, one explicitly. How is this example then different from a normal way of using a control input, i.e. the way that is already covered in examples/simulation

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Sep 22, 2023, 11:47

Commented on doc/examples/simulation/customized_update.rst line 73

It is an important caveat indeed. That's why I put it here and also added a warning that the scheme is no longer fully implicit. I don't see why the particular line you quote is misleading. I merely state that one equation is written in Modelica and the other implemented in the python code. I don't mention anything on how they are discretized at this point. Perhaps I can move the warning higher up.

The control input in examples/simulation is handled in a similar way. The new example in this PR however is narrower in scope and provides details on how the equations are discretized and why users should be careful when adding their own variable updates. I could refer to this example in examples/simulation.

SGeeversAtVortech commented 1 month ago

In GitLab by @k-horvath-deltares on Sep 22, 2023, 14:32

It is indeed different from the normal way of using a control input. This use is needed in several cases, and that is why we needed a clear documentation and example about it.

SGeeversAtVortech commented 1 month ago

In GitLab by @k-horvath-deltares on Sep 22, 2023, 14:32

approved this merge request

SGeeversAtVortech commented 1 month ago

In GitLab by @vreeken on Sep 22, 2023, 18:26

Can you elaborate a bit on how it's different? The Modelica variable is still defined as input, and the value is set with set_var is per usual.

EDIT: If you want to be able to add custom equations (that will be part of the DAE) in Python in simulation (like def constraints in optimization), I suggest building that instead. It'd be somewhat similar to the lookup_tables stuff we talked about, adding a new list of casadi expressions to dae_residual just like delayed_feedback_equations are.

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Sep 26, 2023, 08:37

I think both examples are essentially doing the same thing, but this new example is narrower in scope and explains in more detail what happens and why the user should be careful using it. I could refer to this example in examples/simulation.

SGeeversAtVortech commented 1 month ago

In GitLab by @baayen on Sep 26, 2023, 13:38

This PR needs to be split into PRs for a) the example, and b) the API change.

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Sep 26, 2023, 14:54

marked this merge request as draft

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Sep 26, 2023, 14:55

Yes, I'll make a separate PR of the API change.

SGeeversAtVortech commented 1 month ago

In GitLab by @SGeeversAtVortech on Oct 10, 2023, 12:00

This will be replaced by https://gitlab.com/deltares/rtc-tools/-/merge_requests/462.