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

Clarify clock dependencies requirements #1624

Conversation

clagms
Copy link
Collaborator

@clagms clagms commented Jan 11, 2022

Fixes #1622
Fixes #1620

Claudio Gomes added 2 commits January 11, 2022 10:29
…nly to variables.

Specifies that clock dependencies should be declared in the model structure.
Copy link
Contributor

@andreas-junghanns andreas-junghanns left a comment

Choose a reason for hiding this comment

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

I had to fix a number of things in this PR and it still reads foreign to the text around it due to the mathy-kind of language that introduces symbols just for a single sentence or two. But that and formatting issues aside:

  • A change of a variable value is not the trigger condition for a clock, it is the value (old or new). Corrected.
  • Deactivation of clocks that depend on other activated clocks being deactivated results as a matter of cause - I corrected the language to make this more clear: this is not a special case. Any output value may change because a clocks activation status changed and that needs to be propagated. Clock or not. And the importer must query those values, Clock or not. It might be efficient about it knowing the dependencies, but when ignoring those, it must query all variables.
  • I re-introduced the note about the Clock dependencies. This is non-normative text and it does not contradict the normative text above, as far as I can tell, but clarifies the issue.

As the text is now, I can approve it. If we need to further discuss the changes, let´s.

@clagms
Copy link
Collaborator Author

clagms commented Jan 11, 2022

Thanks for these improvements @andreas-junghanns .
I'd like to get @KarlWernersson's blessing before merging.

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 good to me

@TorstenBlochwitz
Copy link

At least from the changes of this pull request, I do not get the difference in the semantics if a clock is a clocked variable (the attribute "clock" points to another clock) and a clock which has dependencies on other clocks, defined by the attribute "dependencies" in .

I thought #1620 is exactly about this point.

@clagms
Copy link
Collaborator Author

clagms commented Jan 11, 2022

You're right @TorstenBlochwitz .
We need to restrict the clocks attribute to variables that are not clocks.
That way, dependencies between clocks can be placed in the model structure, and clocked partition information can stay in the clocks attribute.

Besides, seeing the clock as a variable that is part of a clocked partition is very difficult to visualize in the mathematical model we provide in the standard. Is the clock also a clocked state? Does it have a previous value?

That also means that the example reintroduced by 3f5011b should be omitted and a sentence needs to be added, making this clear.
EDIT: I made 31d336b to exemplify what I mean.

@andreas-junghanns
Copy link
Contributor

We have been throught this several times in the last 18 months: dependencies carry different semantics from what clocks declare. clocks add more information, namely that accessing variables with clocks attributes is restricted to when these clocks are active. Therefore: We cannot only use the dependencies - they would not be sufficient.
If this is not clear, we should find a good place where to non-normatively explain this.

@andreas-junghanns
Copy link
Contributor

after a discussion with @clagms, I revertes his last commit

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.

My comment was not on restricting anything. We need to clarify which functionalities are enabled by both methods.

…serving the clock and not just activating it.

Also make example more specific to a case that might make more sense in practice.
@clagms
Copy link
Collaborator Author

clagms commented Jan 12, 2022

@andreas-junghanns I think c48b6ef is more in line with what you stated in #1624 (comment)

Please check and merge if you approve.

…about observing the clock and not just activating it."

This reverts commit c48b6ef.
@andreas-junghanns
Copy link
Contributor

Please check and merge if you approve.

Sorry, no, the example was specifically referring to the not-so-obvious case of an input clock depending on another clock. Reverted and will now merge.

@andreas-junghanns andreas-junghanns merged commit bf5d8bd into modelica:master Jan 12, 2022
@clagms
Copy link
Collaborator Author

clagms commented Jan 12, 2022

Sure @andreas-junghanns but you should have left the "observed" word there. It's about restricting observation and not activation 😢

@andreas-junghanns
Copy link
Contributor

observed

Sure @andreas-junghanns but you should have left the "observed" word there. It's about restricting observation and not activation 😢

This sentence is about input clocks. There is no way to observe it (and no need). What am I missing?

@clagms
Copy link
Collaborator Author

clagms commented Jan 13, 2022

So you picked a non obvious example in lieu of a more obvious example (an output clock part of an input clock's model partition), and left out the bit of information that really made it explicit the difference between the clocks attribute and the dependency (the observation restriction). Why not add both?

andreas-junghanns added a commit to andreas-junghanns/fmi-standard that referenced this pull request Jan 13, 2022
@andreas-junghanns
Copy link
Contributor

Because there are descriptions of these trivial examples in the text already: "Output clocks may depend on input Clocks and other variables.".
The difference between "dependencies" and "clocks" needs some clarification. Proposal:
"Note, the dependencies between clocked variables and their Clocks are defined by the attribute clocks in . Compared to dependencies, clocks declared dependencies also restrict when clocked variables can be accessed."
See: 262dfd1

@clagms
Copy link
Collaborator Author

clagms commented Jan 14, 2022

Thanks @andreas-junghanns this is what was missing.

andreas-junghanns added a commit that referenced this pull request Jan 17, 2022
t-sommer pushed a commit to andreas-junghanns/fmi-standard that referenced this pull request Feb 9, 2022
andreas-junghanns added a commit that referenced this pull request Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants