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

Specify no reason for fmi3EnterEventMode #1681

Merged

Conversation

andreas-junghanns
Copy link
Contributor

No description provided.

Copy link
Collaborator

@masoud-najafi masoud-najafi left a comment

Choose a reason for hiding this comment

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

I do not like this solution. In this way, the FMU cannot distinguish between the valid time-event (state-event ) and "I cannot provide" information. Suppose that the importer cannot provide this information and sets timeEvent==True, the FMU desperately looks for a time event and may raise an error. If the importer cannot provide these information, an additional input argument might be better to let the FMU switch to the downgrade mode (old FMI-2.0 detection way that has its uncertainties). switch to down-grade mode can also be done in the instantiateME too. the importer informs the FMU that this feature is not supported and FMU knows that stateEvent and timeEvent information are not reliable.

Copy link
Contributor

@Maplesoft-fmigroup Maplesoft-fmigroup left a comment

Choose a reason for hiding this comment

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

We're still not entirely clear on what the intention of nEventIndicators = 0 is supposed to represent.

@Maplesoft-fmigroup
Copy link
Contributor

Maplesoft-fmigroup commented Feb 17, 2022

In the original wording, it sounded as though the nEventIndicators was supposed to be a count of the number of entries in the part of the model structure. i.e. it is a constant.

In the new wording it sounds as though the value is supposed to be dynamic, and reflect the number of events that occur at the point at which we enter event mode. It also appears as though this is intended to move the burden of event detection from the master to the FMU. If this latter is in fact the goal, this is in direct conflict with the wording "enabling efficient and robust event handling".

Perhaps this needs further discussion?

@andreas-junghanns
Copy link
Contributor Author

Let me try to explain what I think is obvious - or at least should be: rootsFound is an array of length nEventIndicators that informs the FMU which event indicator has a root -- this means that nEventIndicators is just the size of the array rootFound.

How many entries are in rootsFound?
nEventIndicators contains the number of event indicators as deduced from the list <EventIndicator> or 0 if the importer cannot provide this information.
The first part is kind of outdated and only partially true, as FMI 3.0 also has fmi3GetNumberOfEventIndicators, where the explanation reads: The initial value of nEventIndicators is the sum of the sizes of the variables referenced by the EventIndicator elements. The number of state events might change if a variable related to state event has a Dimension that references a structuralParameter.

But we are already clear that nEventIndicators = 0 means the importer has no idea (or does not care enough to share).

@Maplesoft-fmigroup
Copy link
Contributor

Maplesoft-fmigroup commented Feb 17, 2022

Part of our confusion arose from the initial wording that suggested that nEventIndicators should be constant for all time (as it is deduced from the xml file, which is static).

Based on subsequent discussion, it is now clearer that the number of event indicators at a given time can be smaller (but never greater) than the value that could be deduced from the section of the ModelDescription, as some events may deactivate depending on structural parameters. The number of current events can be obtained from fmi3GetNumberOfEventIndicators.

The only remaining issue, aside from clearly communicating the above (assuming we have it right) is why the master would not know the current number of events?
For efficient (or perhaps simply accurate) integration it would not ever want to step too far past an event, so it must be applying some sort of zero crossing detection, and in doing so really ought to know how many zero crossings it is looking for. So the importer shouldn't ever have "no idea" how many events the FMU currently has.

Are we missing something here?

@andreas-junghanns
Copy link
Contributor Author

@masoud-najafi : We introduced fmi3EventQualifier to help with your raised point.
@Maplesoft-fmigroup : Can you please check, if this is now clear?
@t-sommer : watch out: we added an enum and changed the C-API of fmi3EnterEventMode!

Copy link

@TorstenBlochwitz TorstenBlochwitz left a comment

Choose a reason for hiding this comment

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

Overengineered like a German car, but fine.

Copy link
Collaborator

@masoud-najafi masoud-najafi left a comment

Choose a reason for hiding this comment

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

Perfect!

@Maplesoft-fmigroup
Copy link
Contributor

This clarifies things, but we were a bit surprised that in fact the burden of determining zero crossings can be in either the importer or the FMU depending on conditions imposed by the importer.

Since this logic has to be in the FMU anyway (depending on the importer) is there any reason to code the path that uses the rootsFound array? May as well just check in the FMU and ignore these arguments (other than the stateEvent flag that indicates a search is warranted).

@andreas-junghanns
Copy link
Contributor Author

This clarifies things, but we were a bit surprised that in fact the burden of determining zero crossings can be in either the importer or the FMU depending on conditions imposed by the importer.

Since this logic has to be in the FMU anyway (depending on the importer) is there any reason to code the path that uses the rootsFound array? May as well just check in the FMU and ignore these arguments (other than the stateEvent flag that indicates a search is warranted).

While this proposal would the standard simple(r), it does not capture the spirit of FMI: We are trying to get all kinds of exporters and importers enabled to take part. Some tools cannot provide certain information, while others can and allow for more efficient simulations. The "old" FMI 2.0 way was precisely what you propose. But to allow transfer of existing data to reduce repeated work can save significant computation time.

@KarlWernersson
Copy link
Collaborator

KarlWernersson commented Feb 18, 2022

Just so I understand.

FMU's must still support fmi2.0 behavior, is free to ignore this information even if given and must be able to handel events as in FMI 2.0 if no information is given.

If provided the FMU could either

  1. Check that the master and FMU agrees on the event. (I don't see any performance here but good for other reasons)
  2. Accept the events as is, theoretical small performance boost, one source of truth, however I am unsure if I would trust any master with my FMU's in this regard. Anyhow for Dymola case one event could activate other events so some internally check is still needed

I have seen argument's for both 1 and 2 but I don't see how they could be combined, is it up to the FMU to choose in this regard.? Have i missed something else?

PS.
Instead of having a common event function for both ME and CS where most arguments only applies to ME. Is it not cleaner to have two separate enter event mode functions?

@andreas-junghanns
Copy link
Contributor Author

@KarlWernersson : Do you consider this a blocker? Or can you approve the current PR as is?
I want us to tag Beta.4 ASAP and if we keep fiddling around with the API (and then the text also) will postpone this further.
Such delays would need good reason and so far "cleaner" sounds not enough of an argument for further delay (especially as I could make a (weak) counter argument).

@masoud-najafi
Copy link
Collaborator

Also, having two API reduces the flexibility. because a tool may provide TimeEvent but not StateEvent and another tool can provide only StepEvent. Although I do not know why a tool cannot provide all of these information, with this API, any tool can provide as much info as possible.

@KarlWernersson
Copy link
Collaborator

@andreas-junghanns No its not a blocker I will approve.
@masoud-najafi the point is to have separate function for ME and CS not different in ME, as roots and step event is not used in CS. Its not very important to me but we have others that are quite pedantic on how things should look.

Just to make clear.
We have not specified what the FMU should do with this information, which is fine with me but this also implies that the master cannot expect the FMU do do anything with it. This does not prevent the FMU for doing a lot of fancy things to help the master but it could also do nothing.

@andreas-junghanns andreas-junghanns merged commit 8861c6f into modelica:master Feb 18, 2022
@Maplesoft-fmigroup
Copy link
Contributor

The issue we see with this is that the 2 different ways of handling state events either requires 2 completely independent ways of handling events (2 full event implementations in every FMU) or the ability to completely ignore the new way and continue with FMI 2.0 (but requires restrictions on use of the new way).

In the prior version, certain restrictions were detailed on event handling (i.e. the importer needs to step slightly past the event to expect it to fire, etc.) It is unclear whether these restrictions still apply when the importer is the one driving the state events. If they do not, and some importers may view the passing of rootsFound as something that allows one to ignore the restrictions on event firings, then this results in a situation where the event handling is no longer allowed to try to detect events (because it cannot safely do so anymore). This prevents use of the old FMI2.0 method when rootsFound is in use.

This opens up yet another can of worms: What do we do about cases where 'execution' of an event causes a configuration change that results in the necessity of another event firing? (this happens a great deal in models with multiple clutches)
If the importer is the only one allowed to say when a state event occurs, then control will need to return to the importer, only to force the FMU to enter event mode again to handle the next firing. This will result in an efficiency degradation rather than an improvement.

I guess if we clarify that if the importer does not follow all the FMI2.0 rules with respect to firings when rootsFound is used then things will most certainly break then this would allow use of the FMI2.0 way exclusively.
This could then be viewed as an optimization for the first pass of event detection, but it does not buy much (other than saving numberOfEvents comparison operations), as internally after an event executes it will need to compare triggers with the prior iterations anyway, so it will need to evaluate eventIndicators before and after anyhow.

@andreas-junghanns
Copy link
Contributor Author

The issue we see with this is that the 2 different ways of handling state events either requires 2 completely independent ways of handling events (2 full event implementations in every FMU) or the ability to completely ignore the new way and continue with FMI 2.0 (but requires restrictions on use of the new way).

Yes, this was also @KarlWernersson´s criticism: If you can´t be sure the importer will tell you, you have to assume you need to detect this events yourself. However, with the new, three-state qualifier, the importer can tell you. You still have to package the event detection code, but good importer implementations will help you skip the effort of event detection.
This is very similar to many other features in FMI: We enable sophisiticated use cases, but do not require sophisticated handling.

In the prior version, certain restrictions were detailed on event handling (i.e. the importer needs to step slightly past the event to expect it to fire, etc.) It is unclear whether these restrictions still apply when the importer is the one driving the state events. If they do not, and some importers may view the passing of rootsFound as something that allows one to ignore the restrictions on event firings, then this results in a situation where the event handling is no longer allowed to try to detect events (because it cannot safely do so anymore). This prevents use of the old FMI2.0 method when rootsFound is in use.

As far as I know, this should still be the same: The importer must step slightly past the event because only then the rootsFound vector is consistent with the state of the model. If this needs clarification, please suggest where.
If I missed that this was a delibarate change, we absolutely need to make that clear also.

This opens up yet another can of worms: What do we do about cases where 'execution' of an event causes a configuration change that results in the necessity of another event firing? (this happens a great deal in models with multiple clutches) If the importer is the only one allowed to say when a state event occurs, then control will need to return to the importer, only to force the FMU to enter event mode again to handle the next firing. This will result in an efficiency degradation rather than an improvement.

I am not an event handling specialist. But I venture to say, that the clutch-triggering example is exactly why we have super-dense time steps to figure out exactly these dependencies. And the mechanisms there should be clear, see fmi3UpdateDiscreteStates and especially discreteStatesNeedUpdate = fmi3True.

@KarlWernersson
Copy link
Collaborator

In the prior version, certain restrictions were detailed on event handling (i.e. the importer needs to step slightly past the event to expect it to fire, etc.) It is unclear whether these restrictions still apply when the importer is the one driving the state events. If they do not, and some importers may view the passing of rootsFound as something that allows one to ignore the restrictions on event firings, then this results in a situation where the event handling is no longer allowed to try to detect events (because it cannot safely do so anymore). This prevents use of the old FMI2.0 method when rootsFound is in use.

As the FMI 2.0 version must always be supported and there is no guarantee that the FMU does anything with the roots found
I don't think the master can assume anything like that. A clarification would be nice.

This opens up yet another can of worms: What do we do about cases where 'execution' of an event causes a configuration change that results in the necessity of another event firing? (this happens a great deal in models with multiple clutches)
If the importer is the only one allowed to say when a state event occurs, then control will need to return to the importer, only to force the FMU to enter event mode again to handle the next firing. This will result in an efficiency degradation rather than an improvement.

This is very likely to happen for modelica models as you state. However these roots are only given when entering event mode, so successive roots are solved internally either directly in the first call to fmi3UpdateDiscreteStates or by a new function call signaled by a return value of discreteStatesNeedUpdate = true from that function.

I guess if we clarify that if the importer does not follow all the FMI2.0 rules with respect to firings when rootsFound is used then things will most certainly break then this would allow use of the FMI2.0 way exclusively.
This could then be viewed as an optimization for the first pass of event detection, but it does not buy much (other than saving numberOfEvents comparison operations), as internally after an event executes it will need to compare triggers with the prior iterations anyway, so it will need to evaluate eventIndicators before and after anyhow.

There is also the case of confirming that the FMU and the master agrees on the state event.

What needs to be clarified, (For beta 5 or RC1)
FMU is free to do what it want with the provided information so the master cannot assume that the FMU does anything with it (i.e FMI 2.0)

maybe a non normative text. in this style?

[What the FMU does with this information is unspecified so the master cannot assume that it will do anything.]

@KarlWernersson
Copy link
Collaborator

KarlWernersson commented Feb 21, 2022

Yes, this was also @KarlWernersson´s criticism: If you can´t be sure the importer will tell you, you have to assume you need to detect this events yourself. However, with the new, three-state qualifier, the importer can tell you. You still have to package the event detection code, but good importer implementations will help you skip the effort of event detection.
This is very similar to many other features in FMI: We enable sophisiticated use cases, but do not require sophisticated handling.

FMU should still be able to ignore this info even if the master can provide it.

Edit: Maybe this is what you said i just wanted to make it very clear

@andreas-junghanns andreas-junghanns deleted the ReasonEnterEventMode branch February 21, 2022 10:44
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.

EnterEventMode definition needs clarification
5 participants