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

Handle time events in model exchange example. #71

Merged
merged 20 commits into from
May 7, 2021

Conversation

clagms
Copy link
Collaborator

@clagms clagms commented Sep 23, 2020

This fixes the problem exemplified in #70 and described in #72

@andreas-junghanns and @t-sommer, please note that this is a serious problem because this code is being given as an example in the FMI standard document.

It should now pass all the checks, though I had to restrict the new FMU to ME of FMI 3.0, because it is very difficult to get the correct reference behavior from FMPy (in the case of co-simulation), which is being used as ground truth.

…nto handle_time_events

# Conflicts:
#	examples/model_exchange.c
@clagms clagms marked this pull request as ready for review April 24, 2021 04:11
Copy link
Collaborator

@t-sommer t-sommer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to introduce a new model? The "Stairs" model already has time events.

@clagms
Copy link
Collaborator Author

clagms commented Apr 26, 2021

Why is it necessary to introduce a new model? The "Stairs" model already has time events.

@t-sommer , the Stairs model is not being used with the model exchange example that is shown in the standard. The new model exemplifies both state and time events, and how state events can affect the next time of time events.

What concrete changes do you propose? We could replace the BouncingBall example, as the new example supersedes that one.

@t-sommer
Copy link
Collaborator

I'll put the simulation loop in a separate file, so we can reuse it for all models.

@clagms
Copy link
Collaborator Author

clagms commented Apr 26, 2021

I'll put the simulation loop in a separate file, so we can reuse it for all models.

That sounds good! I guess you have access to this PR, right? I made sure all maintainers had access.

@clagms
Copy link
Collaborator Author

clagms commented Apr 26, 2021

Also note that the time event detection should still be exact, and not restricted to the fixed step size used by the solver in model exchange. I understand that one might wish to fix it to the step size used (as done in #99 ) as this facilitates output printing and all that, but it's wrong: time events occur at a specific time and not subject to the whims of the solver. If one wants to output at fixed intervals, then interpolation should be used.

@t-sommer
Copy link
Collaborator

The same is true for root finding, but requires a variable step solver (e.g. CVode).

@clagms
Copy link
Collaborator Author

clagms commented Apr 26, 2021

The same is true for root finding, but requires a variable step solver (e.g. CVode).

I don't fully agree that these two are the same. With state events, we know don't know up front the time they occur (on this, both FMU and Importer agree), so it's ok to tolerate inaccuracies caused by step size. On time events, the FMU dictates when the next one should occur, and the Importer should do its best to get there. The implementation of timed event handling is also different than the state event handling (though state event handling supersedes time event handling, in a "killing a fly with a cannon" way).

But I see the appeal of treating the two in the same way :)

@t-sommer
Copy link
Collaborator

t-sommer commented May 6, 2021

@clagms, with your permission I will merge this PR.

@clagms
Copy link
Collaborator Author

clagms commented May 7, 2021

@clagms, with your permission I will merge this PR.

All good with me!

@clagms clagms merged commit c8c9b87 into modelica:master May 7, 2021
@clagms clagms deleted the handle_time_events branch July 12, 2021 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants