-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add event check and update for ME2 #109
base: master
Are you sure you want to change the base?
Conversation
@@ -351,7 +351,9 @@ def __init__(self, | |||
event_info = self.model.get_event_info() | |||
if event_info.upcomingTimeEvent and event_info.nextEventTime == model.time: | |||
self.model.event_update() | |||
|
|||
elif isinstance(self.model, fmi.FMUModelME2): | |||
self.model.event_update() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the event_info condition ? I guess its there to avoid updating at each timestep
I see fmi2 also has event_info but upcomingTimeEvent is named nextEventTimeDefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jschueller! And really sorry for my delay here. But after some further testing compiling my test fmu (from #72) as an ME1 instead of ME2, it seems the if
condition in Line 352 is also insufficient to detect an event. I'm finding that event_info.upcomingTimeEvent
always returns false and event_info.nextEventTime
always returns 0.0. Similarly, if applied to ME2, event_info.nextEventTimeDefined
always returns false and event_info.nextEventTime
always returns 0.0. Therefore, in order to get my expected performance, the self.model.event_update()
needs to be called as I suggest, without the if
condition on event_info
, in both the ME1 and ME2 cases. I'm still not sure if I'm missing something here though.
@dhblum Hi, I am sorry for the lack of replies. Do you know if this is still an issue? I am trying to look into the current issues and close obsolete ones and mark still active ones. |
Thanks @modelonrobinandersson. As far as I can tell, yes this is still an issue, as in the master branch the section starting at https://github.com/modelon-community/PyFMI/blob/master/src/pyfmi/fmi_algorithm_drivers.py#L390 still only checks if there's an event if FMU is ME1 but not if ME2. |
Thank you @dhblum for the update. I will have to look into the FMI spec first and figure out if this is intended or a miss. I will get back to you as soon as I know more but it will likely be later in August. |
Thank you @modelonrobinandersson for your attention to this, sounds good. |
Hey @dhblum, sorry for getting back to this a bit late. Do you have a test model in mind where this change seems to be necessary? |
@modelonrobinandersson the test case can be found attached in #72 if I'm not mistaken |
Thank you, I will take a look. |
Thank you @modelonrobinandersson for taking a look. |
Hi @dhblum sorry for the late reply, I've not had time until just now. I have reproduced the issue on my end as well, and it does indeed look fishy. I will try to do some more testing during the week to see if there are any implications with the proposed fix. |
Alright, thanks so much @modelonrobinandersson. |
Hi @dhblum! I have now looked into it further. The good news is that it is working as expected if you try to simulate the full time directly (instead of advancing step by step). See below:
The reason why it does not work the way you have done in your script is because there is currently an issue where we need to detect if there are state events when the simulation starts. This is not trivial to fix. I don't have any time for when we can look into this again except for some time again in the future. |
doesnt this PR actually fix the issue ? |
Hi. Unfortunately it does not because we only want to do the event update if there are state events, the PR however forces event updates every time the simulation starts which is incorrect. Moreover two of the PyFMI tests that are working as expected fail with the the PR. |
ok, thanks for the explanation |
Howdy, @modelonrobinandersson , I'm wondering if i meet the same issue as you said. I run the simulation with multiple times. Every time the simulation would be started with a fmu_set_state (last state). And i found my time-changing signal cannot have an influence on my system. The interesting stuff is that, I also changed my input at the start of the simulation time, which means there will be a state event when the simulation starts. The details could be found in #159. Now I hope this will be the reason for my issue. I will make this state event not happen at the start of the simulation time. If you have any sugguestions, please let me know. I appreciate any hints. Thank you very much. |
I did some tests and found my problem is exactly what @modelonrobinandersson said. There will be some problems when the simulation period starts with a state event. What I did to bypass this problem was to keep the previous state of my input for two time steps and then change the input for the following simulation period (I run multiple simulation periods). It works well! The following figure shows the difference between the original situation with the issue and the situation with my trick. I compared the results with Dymola. Even though there were still some gaps, the trend looked similar and nice. BTW, I'm wondering if Modelon will fix this issue in the future @modelonrobinandersson since PyFMI is a really useful library, and I have used it for a long time. Thanks for your discussion, which inspired me a lot! @dhblum @modelonrobinandersson |
Hi @lambtt, sorry for my late reply I just discovered I have not replied. We will fix this issue at some point but currently we haven't been able to prioritize it. It also seems that this issue is quite rare as it is not commonly appearing in most FMUs. Again, sorry for the very late response. |
@modelonrobinandersson Hey everyone, I have been using pyfmi for a while and arrived at the same Functional Mockup Unit (FMU) issue on this pull request. Several people on my team and I are working on “model predictive controls” with FMUs, which requires us to take the outputs of an FMU at the current time step, feed them into a “model predictive control” algorithm, and return the desired inputs into the FMU at the next time step (while maintaining the states of the FMU at every time step). Therefore, being able to advance step by step with the pyfmi/FMU simulation while maintaining the FMU states would be very helpful for our “model predictive control” algorithm development, as compared to simulating the FMU for the full time. I appreciate the efforts of everyone to identify this issue, and hope this bug can be prioritized soon:) |
This is for #72.
In particular, it invokes an event update for an ME2 model at the start time of a simulation. Currently, it looks like this is only done for ME1.