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

Detailed when #3532

Merged
merged 8 commits into from
Sep 18, 2024
Merged

Detailed when #3532

merged 8 commits into from
Sep 18, 2024

Conversation

HansOlsson
Copy link
Collaborator

Closes #3499
Closes #3498

@HansOlsson HansOlsson added this to the 2024-June milestone Jun 23, 2024
chapters/equations.tex Outdated Show resolved Hide resolved
chapters/equations.tex Outdated Show resolved Hide resolved
chapters/equations.tex Outdated Show resolved Hide resolved
chapters/functions.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

In addition to other suggestions, I'd like to get rid of this non-normative text, now that we have clearly defined that it is not allowed:

\begin{nonnormative}
Comment: The semantics are undefined if the function call of an
impure function is part of an algebraic loop.
\end{nonnormative}

@HansOlsson
Copy link
Collaborator Author

Phone meeting:
Hans:

If we want to have names:

  1. Initialization, with additional variables (pre(v) as unknown and x for states) and additional equations (initial equations etc).
  2. (Initial) Event iteration; iteration procedure at initial event
  3. Start of continuous integration
    But probably no need to add.

Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

The request to remove obsolete text in #3532 (review) is still not addressed. Apart from that, it looks OK.

Co-authored-by: Henrik Tidefelt <[email protected]>
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

Same as last time:

The request to remove obsolete text in #3532 (review) is still not addressed. Apart from that, it looks OK.

If there is a reason why this text shouldn't be considered obsolete, please explain.

@HansOlsson
Copy link
Collaborator Author

HansOlsson commented Jul 2, 2024

Same as last time:

The request to remove obsolete text in #3532 (review) is still not addressed. Apart from that, it looks OK.

If there is a reason why this text shouldn't be considered obsolete, please explain.

There are several reasons to keep that:

  1. It wasn't part of PR - this constant scope creep of blocking PR for parts that aren't in the PR needs to stop.
  2. Since there are ways of circumventing the purity-check it is possible to call an impure function in a system of equations without it being an error.

@HansOlsson
Copy link
Collaborator Author

Pinging for reviews after holidays.

@HansOlsson HansOlsson modified the milestones: 2024-June, 2024-September Aug 30, 2024
@henrikt-ma
Copy link
Collaborator

henrikt-ma commented Sep 5, 2024

  • It wasn't part of PR - this constant scope creep of blocking PR for parts that aren't in the PR needs to stop.

It is related to this PR, as this PR is introducing this rule that looks crystal clear to me:

It is an error if an impure function call is part of a systems of equations (including linear systems), even if called in agreement with the restrictions above.

@HansOlsson
Copy link
Collaborator Author

  • It wasn't part of PR - this constant scope creep of blocking PR for parts that aren't in the PR needs to stop.

It is related to this PR, as this PR is introducing this rule that looks crystal clear to me:

It is an error if an impure function call is part of a systems of equations (including linear systems), even if called in agreement with the restrictions above.

I have opened a different PR #3571 to clarify that, since as previously stated it is an unrelated issue.

Copy link
Member

@eshmoylova eshmoylova left a comment

Choose a reason for hiding this comment

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

Checking with the initialization team for any feedback.

chapters/functions.tex Outdated Show resolved Hide resolved
Copy link
Member

@eshmoylova eshmoylova left a comment

Choose a reason for hiding this comment

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

Changes look good overall. Just requesting a typo fix.

Co-authored-by: Elena Shmoylova <[email protected]>
@HansOlsson HansOlsson dismissed henrikt-ma’s stale review September 18, 2024 12:19

Other text is part of a separate PR

@HansOlsson HansOlsson merged commit 33a0c5f into modelica:master Sep 18, 2024
1 check passed
@HansOlsson HansOlsson deleted the DetailedWhen branch September 18, 2024 12:19
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.

when initial() with impure function? when equations in algebraic loop?
3 participants