Skip to content

Conversation

@HansOlsson
Copy link
Collaborator

Closes #3091

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 couldn't spot any obvious mistakes, but I think we need a reviewer with a deeper understanding of the chapter.

@henrikt-ma
Copy link
Collaborator

By the way, after this PR is merged, I suggest that we fix the remaining non-sentence-based line breaks in these chapters. It's too late to do it when one wants to make a change like this PR, as it would have become too hard to spot the actual changes of terminology among all white-space changes.

@HansOlsson
Copy link
Collaborator Author

Would be good with a review of this.

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 only problem I could spot was that a discretized partition is still sometimes referred to as a clocked partition.

@henrikt-ma
Copy link
Collaborator

Please also resolve merge conflicts.

@HansOlsson
Copy link
Collaborator Author

Please also resolve merge conflicts.

Now resolved.

@HansOlsson HansOlsson added this to the Phone 2023-3 milestone Jun 5, 2023
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 find this sentence strange:

From the view of the clocked partition, the continuous-time partition is discretized and the discretized continuous-time variables have only a value at a clock tick.
  • I am missing the reference in the clocked partition, suggesting that we say a clocked partition instead.
  • Are we really only speaking of the clocked partitions here, excluding the discretized partitions? (Since the meaning of clocked partition is being changed, the meaning of this sentence has also been changed.)

@HansOlsson
Copy link
Collaborator Author

From the view of the clocked partition, the continuous-time partition is discretized and t...

That should just be other partitions.

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 am generally totally in favor of this cleanup, but it is a very tricky PR to review. Not only should it be reviewed by an expert on the synchronous features (which I am not), but to make a really thorough review (which I haven't) of just the implementation of the new terminology, one would need to read the entire chapter carefully in its new form.

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.

Looks good overall. Some uncertainties, but I don't want to mark them as "request changes" so adding the review as "Comment."

@HansOlsson HansOlsson requested a review from henrikt-ma January 12, 2024 14:14
@HansOlsson
Copy link
Collaborator Author

I think all comments have now been handled.

@HansOlsson
Copy link
Collaborator Author

@henrikt-ma can you review this one, or dismiss your review in some way if you don't find it relevant?

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.

Just pointing out two very minor things to fix this time around. After that, I plan to approve based on the gut feeling that this is now so good that it is a clear overall improvement over the old state, even if I can't convince myself that I have a firm grip on the effect on the entire chapter.

@HansOlsson HansOlsson requested a review from henrikt-ma January 24, 2024 12:06
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.

After all these iterations I've at least convinced myself that this hard-to-review PR takes the chapter to a better state than it was before. Once we've become used to the new terminology, it will probably be easier to detect any remaining mistakes in its application.

@HansOlsson HansOlsson merged commit 4860f2f into modelica:master Feb 1, 2024
@HansOlsson HansOlsson deleted the Discretized branch February 1, 2024 09:15
@HansOlsson HansOlsson added the M37 For pull requests merged into Modelica 3.7 label Feb 1, 2024
@HansOlsson HansOlsson added clarification Specification of feature is unclear, but not incorrect semanticChanges For pull request that changes the semantics (neither used for fixes nor enhancements). labels Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clarification Specification of feature is unclear, but not incorrect M37 For pull requests merged into Modelica 3.7 semanticChanges For pull request that changes the semantics (neither used for fixes nor enhancements).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename and clarify clocked discretized continuous-time partition

3 participants