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

Provide better alternative. #3144

Merged
merged 3 commits into from Apr 26, 2022
Merged

Conversation

HansOlsson
Copy link
Collaborator

Closes #3143
Having examples that we know lead to numerical issues isn't a good idea, especially as there's an equivalent formulation without the problems - and we know that many just copy the examples from the specification as a starting point.

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.

While I didn't see a strict need to change anything in view of the low frequency of the zero crossing sine, I don't see a problem with using change(revolutions) either.

My only request for change is that we then also remove the last remaining trace of the sine zero crossing.

chapters/synchronous.tex Outdated Show resolved Hide resolved
White space

Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
chapters/synchronous.tex Outdated Show resolved Hide resolved
Remove note about old.

Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
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.

I don't really have anything to object to, but I note that all of this came out of an interesting discussion about high frequency zero crossing functions, and the proposed change is only sweeping that under the rug. On the other hand, as the topic is a difficult numerical issue, I don't want to insist on trying to find the right formulations to put in the specification either.

@HansOlsson
Copy link
Collaborator Author

My only request for change is that we then also remove the last remaining trace of the sine zero crossing.

That is now done.

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.

My only request for change is that we then also remove the last remaining trace of the sine zero crossing.

That is now done.

Yes, I noted, and that's why I only have a comment regarding the potential loss of the interesting discussion about problems with high frequency signals in zero crossings. As long as there's some sort of agreement that we don't want to add anything regarding that, I'm fine with the current state.

@HansOlsson
Copy link
Collaborator Author

Yes, I noted, and that's why I only have a comment regarding the potential loss of the interesting discussion about problems with high frequency signals in zero crossings. As long as there's some sort of agreement that we don't want to add anything regarding that, I'm fine with the current state.

Either we don't add anything - or we add it more generally for crossing functions.

@HansOlsson HansOlsson added this to the Phone 2022-2 milestone Apr 21, 2022
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.

Approving, as there didn't seem to be that much interest in adding some general remark about the difficulty to handle high frequency zero crossing functions.

@HansOlsson HansOlsson merged commit 29d7587 into modelica:master Apr 26, 2022
@HansOlsson HansOlsson deleted the ImproveClockExample branch April 26, 2022 07:22
@HansOlsson HansOlsson added the M36 For pull requests merged into Modelica 3.6 label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M36 For pull requests merged into Modelica 3.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad Event clock examples
3 participants