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

Let <EventIndicator> inherit from fmi3Unknown #1377

Merged
merged 2 commits into from
Apr 15, 2021

Conversation

t-sommer
Copy link
Collaborator

@t-sommer t-sommer commented Mar 27, 2021

@andreas-junghanns
Copy link
Contributor

@t-sommer : what is missing here for review? Can I help?

@t-sommer
Copy link
Collaborator Author

The schema figures still need to be regenerated.

@t-sommer t-sommer marked this pull request as ready for review March 29, 2021 08:06
@KarlWernersson
Copy link
Collaborator

Dont we get dependecies and dependenciskind if we do this?
I don't think we want that.

I am quite sure float32 is not supported for event indicators just as it is limited for derivatives states etc?

Copy link
Collaborator

@KarlWernersson KarlWernersson left a comment

Choose a reason for hiding this comment

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

See previous comment

@KarlWernersson
Copy link
Collaborator

@t-sommer @masoud-najafi @TorstenBlochwitz @CSchulzeTLK

Hi

I just want to make sure that we all understand the changes we make and its implications before we implement them, as I did not understand that at the start of the discussion.

I am no longer contesting the need to expose event indicators. And if I understand the use case for that was additional debug information.
Because of this when it was first implemented we explicitly say in the standard that we don’t have dependencies (unless this was a bug?)

Now there is a proposal to add the dependencies to event indicators as this will uniform model structure. I agree that this goal is good but we need to go over the implications before we do the change. If it is just for debugging, do we want all these extra features? Or will we expand the use case?
If we do not want these features should we put the information somewhere else instead of in the modelStructure.

  • GetDirectional/Adjoint Derivative, for all other variables in modelStructrue getDireciotnalDerivtatves is allowed at some state. If we allow this for modelStructre it requires any exporter that supports this functionality to also support it for eventIndicatros. The state is is allowed should be listed in 2.2.11 my assumption would be that we allow it during continuous time but disallow during event mode.
    If do not want to allow this it needs to be explained why.

  • Should they be included in Initial Unknowns. Then we need to update table 24 as well
    -- I would say yes if we go for dependencies

  • can they have a fixed start value?
    -- I do not see a reason to forbid it

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.

Looks OK.

@chrbertsch
Copy link
Collaborator

chrbertsch commented Apr 14, 2021

What was the conclusion at the regular design meeting on this PR? Could someone please summarize?

@chrbertsch chrbertsch added this to the v3.0beta.1 milestone Apr 14, 2021
@andreas-junghanns
Copy link
Contributor

What was the conclusion at the regular design meeting on this PR? Could someone please summarize?

We accept this PR, but need to extend it by some restrictions w.r.t. derivative computations for EventIndicators.

@KarlWernersson will provide some wording with a pointer where to put it in this issue as comment and I will add this to the PR.

@KarlWernersson
Copy link
Collaborator

@andreas-junghanns
reading the document I am not compliantly sure on were to put such a restriction.
As we are now explicitly stating which variables can be retried at which state by defining unknowns at 2.2.11. Getting Partial Derivatives

However to avoid confusion I would recommend a remark something in this style..

_Note that Event indicators are not included among the unknowns. This is intentional as their dependences in modelStructure are intended for debugging purposes and not for partial derivatives. _

@KarlWernersson
Copy link
Collaborator

A similar remark could be put in InitialUnknowns 2.4.8 table 24

Event indicators are available during initialization mode however since their dependencies are intended for debugging purposes and not for connection with other FMU's they should not be listed among the initial unknowns

@andreas-junghanns
Copy link
Contributor

Event indicators are available during initialization mode however since their dependencies are intended for debugging purposes and not for connection with other FMU's they should not be listed among the initial unknowns

Please check if my changes reflect your intendions...

@andreas-junghanns
Copy link
Contributor

Note to self:
We need to merge master back into this branch and regenerate images before merging back into master.

Copy link
Collaborator

@KarlWernersson KarlWernersson left a comment

Choose a reason for hiding this comment

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

Looks great thanks =)

@andreas-junghanns
Copy link
Contributor

andreas-junghanns commented Apr 15, 2021

t-sommer: Can you merge the XSD changes, regenerate the images and then merge into master? Please?

@t-sommer t-sommer merged commit e42cd00 into modelica:master Apr 15, 2021
@t-sommer t-sommer deleted the event-indicator-unknown branch April 15, 2021 16:09
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.

5 participants