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

Move Clock dependency info to attribute #1375

Merged
merged 2 commits into from
Apr 1, 2021

Conversation

t-sommer
Copy link
Collaborator

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

@andreas-junghanns
Copy link
Contributor

@t-sommer : Is this ready for review? If not, can I help?

@TorstenBlochwitz : Can you have a look, please and comment? Do you remember why we originally chose to have a ModelStructure element and not add this as attribute to the Variable element as now proposed?

@KarlWernersson
Copy link
Collaborator

@andreas-junghanns
As far as I remember, the discussion was that both ways of of representing the information was valid and gave the same information. It ended up being a matter of preference on how to represent it. A vote was cast and it ended 2:1, it the small working group.

@t-sommer
Copy link
Collaborator Author

The schema figures still need to be regenerated.

@andreas-junghanns
Copy link
Contributor

@andreas-junghanns
As far as I remember, the discussion was that both ways of of representing the information was valid and gave the same information. It ended up being a matter of preference on how to represent it. A vote was cast and it ended 2:1, it the small working group.

@t-sommer convinced me that this proposal might be a better way for these reasons:

  • shorter lists are more human readable
  • information is now more naturally aligned with other "Variable" information
  • ModelStructure uses the other dependency direction for (most) other elements

@TorstenBlochwitz : Any strong arguments against?

@andreas-junghanns
Copy link
Contributor

The schema figures still need to be regenerated.

Done.

@andreas-junghanns
Copy link
Contributor

Ready for review?

@t-sommer t-sommer marked this pull request as ready for review March 26, 2021 09:23
@IZacharias
Copy link
Collaborator

I like the idea of putting the clock dependency back to the variables.

But why are we limiting this to discrete variables?
Especially in Scheduled Execution, we need the relationships to clocks not only for inputs and outputs, but also for parameters and local variables. Only in this way the master is able to generate optimal code. The master needs the information to decide on where to place the calls to the fmi3GetXXX() and fmi3SetXXX() functions.
Example: If a parameter is only needed for the calculation of exactly one model partition, updating its value, i.e. calling the fmi3SetXXX() function, is only necessary and meaningful before calling the fmi3ActivateModelPartition() function for the corresponding clock.

@KarlWernersson
Copy link
Collaborator

@IZacharias

But why are we limiting this to discrete variables?

Only discrete variables will change at a clock tick.
If it is continuous it cannot belong to the clock.

For fixed tunable and constant. These are set or retrieved independent of clock ticks.
You would only call set/get at initializatoinMode (possible event mode if you change tunable).
So is it really needed for optimizing that? Initialization is still done globally and not on specific clock partion.

But it could still have some use for model information to assign parameters to specific clock partitions, so as long as we forbid continuous I am ok to allow all others to be part of a clock.

@IZacharias
Copy link
Collaborator

@IZacharias

But why are we limiting this to discrete variables?

Only discrete variables will change at a clock tick.
If it is continuous it cannot belong to the clock.

For fixed tunable and constant. These are set or retrieved independent of clock ticks.
You would only call set/get at initializatoinMode (possible event mode if you change tunable).
So is it really needed for optimizing that? Initialization is still done globally and not on specific clock partion.

But it could still have some use for model information to assign parameters to specific clock partitions, so as long as we forbid continuous I am ok to allow all others to be part of a clock.

@KarlWernersson You're right, I probably didn't make that clear enough. My point is to allow it for tunable parameters, discrete inputs, discrete outputs, and discrete local variables. For fixed, constant, continuous it is not needed or makes no sense.

@chrbertsch
Copy link
Collaborator

Regular Design meeting:

let's merge it. @t-sommer will do so.

[[clocks,`clocks`]]
If present, this variable is clocked.
The value of <<clocks>> is a list of value references of <<Clock>> variables this variable depends on.
Only discrete variables can have this attribute.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only discrete variables can have this attribute.

We need the information also for tunable parameters, i.e. discrete variables and tunable parameters can have this attribute.

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.

clock dependency definition in model structure
5 participants