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

Add event check and update for ME2 #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dhblum
Copy link

@dhblum dhblum commented Jun 9, 2021

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.

@@ -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()
Copy link
Contributor

@jschueller jschueller Jun 10, 2021

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

Copy link
Author

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 ifcondition on event_info, in both the ME1 and ME2 cases. I'm still not sure if I'm missing something here though.

@modelonrobinandersson
Copy link
Collaborator

@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.

@dhblum
Copy link
Author

dhblum commented Jul 28, 2022

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.

@modelonrobinandersson
Copy link
Collaborator

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.

@dhblum
Copy link
Author

dhblum commented Jul 29, 2022

Thank you @modelonrobinandersson for your attention to this, sounds good.

@modelonrobinandersson modelonrobinandersson added the question Issue only concerns a or multiple questions. label Jul 29, 2022
@modelonrobinandersson
Copy link
Collaborator

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?

@jschueller
Copy link
Contributor

@modelonrobinandersson the test case can be found attached in #72 if I'm not mistaken

@modelonrobinandersson
Copy link
Collaborator

@modelonrobinandersson the test case can be found attached in #72 if I'm not mistaken

Thank you, I will take a look.

@dhblum
Copy link
Author

dhblum commented Sep 13, 2022

Thank you @modelonrobinandersson for taking a look.

@modelonrobinandersson
Copy link
Collaborator

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.

@dhblum
Copy link
Author

dhblum commented Oct 3, 2022

Alright, thanks so much @modelonrobinandersson.

@modelonrobinandersson
Copy link
Collaborator

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:

    u1 = [0, 0, 0.7, 0.7, 0.1]
    u2 = [0, 0, 2, 2, 0.5]
    u3 = [0, 0, 3.3, 3.3, 2.3]
    inputs = (u1, u2, u3)
    time = np.array([0, 100, 100, 200, 200])
    input_object = (['u1','u2', 'u3'], np.transpose(np.vstack((time, inputs[0], inputs[1], inputs[2]))))
    fmu_path = compile_fmu('eventTest', 'eventTest.mo', target='me')
    fmu = load_fmu(fmu_path)
    res = fmu.simulate(0, 300, input = input_object) 

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.

@jschueller
Copy link
Contributor

jschueller commented Oct 7, 2022

doesnt this PR actually fix the issue ?

@modelonrobinandersson
Copy link
Collaborator

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.

@modelonrobinandersson modelonrobinandersson added bug Issue or PR covers a bug. and removed question Issue only concerns a or multiple questions. labels Oct 7, 2022
@modelonrobinandersson modelonrobinandersson self-assigned this Oct 7, 2022
@jschueller
Copy link
Contributor

ok, thanks for the explanation

@lambtt
Copy link

lambtt commented Dec 12, 2022

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.

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.

@lambtt
Copy link

lambtt commented Dec 12, 2022

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.

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.

image

I compared the results with Dymola. Even though there were still some gaps, the trend looked similar and nice.

image

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

@modelonrobinandersson
Copy link
Collaborator

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.
I also saw your comment in #159 (comment) and I will therefore close issue #159 as I have now responded here.

Again, sorry for the very late response.

@wehuang16
Copy link

wehuang16 commented Jun 29, 2024

@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:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue or PR covers a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants