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.

Sign up for GitHub

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

Jump to bottom

MSL v4.1.0-beta.1 feedback: Fix comparison signals #4304

Merged
merged 1 commit into from
Feb 16, 2024
Merged

MSL v4.1.0-beta.1 feedback: Fix comparison signals #4304

merged 1 commit into from
Feb 16, 2024

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Feb 10, 2024 •
edited
Loading

This an accumulated PR of all my findings when running the ReleaseChecks

  • Remove empty lines in comparisonSignals.txt
  • Remove unreferenced comparisonSignals.txt
  • Fix signals names in comparisonSignals.txt (or rename in example model)
  • Rename DemoSomething to DemonstrateSomething consistently (for all newly introduced example models)

@beutlich beutlich added L: Resources Issue addresses Modelica/Resources (excl. C-Sources) example Issue only addresses example(s) V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases) labels Feb 10, 2024
@beutlich beutlich added this to the MSL4.1.0 milestone Feb 10, 2024
@henrikt-ma
Copy link
Contributor

  • Rename DemoSomething to DemonstrateSomething consistently (for all newly introduced example models)

If we're in the process of renaming things, why don't just drop the Demonstrate prefix from a package name under Examples?

@HansOlsson
Copy link
Contributor

This an accumulated PR of all my findings when running the ReleaseChecks

  • Remove empty lines in comparisonSignals.txt
  • Remove unreferenced comparisonSignals.txt
  • Fix signals names in comparisonSignals.txt (or rename in example model)
  • Rename DemoSomething to DemonstrateSomething consistently (for all newly introduced example models)

I don't consider using Demonstrate instead of Demo as an improvement; basically 'Demo' is well-known, even more used than Demonstrate and here it can also be seen as a noun, as a Demo(nstration) of Something.

@beutlich
Copy link
Member Author

beutlich commented Feb 12, 2024 •
edited
Loading

I have no strong opinion except than fixing the naming inconsistency and broken signalComparisons.txt that we have now. Feel free to push on this PR branch or file an alternative PR.

AHaumer added a commit to AHaumer/ModelicaStandardLibrary that referenced this pull request Feb 13, 2024
@AHaumer
resolves modelica#4304
68af908
@casella casella closed this in 1eb156b Feb 13, 2024
@beutlich
Copy link
Member Author

Hm, seems to be closed by accident.

@beutlich beutlich reopened this Feb 13, 2024
Harisankar-Allimangalath pushed a commit to Harisankar-Allimangalath/ModelicaStandardLibrary_1 that referenced this pull request Feb 15, 2024
@AHaumer @Harisankar-Allimangalath
resolves modelica#4304 (modelica#4311)
5cac4ab
@beutlich
Copy link
Member Author

@HansOlsson @henrikt-ma How to proceed? This PR holds some necessary fixes for the comparisonSignals.txt, but also some optional fixes for example naming.

@HansOlsson
Copy link
Contributor

@HansOlsson @henrikt-ma How to proceed? This PR holds some necessary fixes for the comparisonSignals.txt, but also some optional fixes for example naming.

I don't know. I would split off the Demo-naming part and just merge the rest now, and then discuss Demo.

@maltelenz
Copy link
Contributor

@HansOlsson @henrikt-ma How to proceed? This PR holds some necessary fixes for the comparisonSignals.txt, but also some optional fixes for example naming.

I don't know. I would split off the Demo-naming part and just merge the rest now, and then discuss Demo.

Agreed, this could get us the uncontroversial fixes faster.

@AHaumer
Copy link
Contributor

AHaumer commented Feb 15, 2024

I'd be happy if you split this into 2 parts:

  • change naming: IMHO unnecessary
  • reference results
    I don't understand: Is this fixing problems with some (not all) reference results, or just creating new ones?
    If it's the second, I disagree (because that counteracts the idea of regression testing).

@beutlich beutlich changed the title MSL v4.1.0-beta.1 feedback: Fix comparison signals and rename Demo* to Demonstrate* MSL v4.1.0-beta.1 feedback: Fix comparison signals Feb 15, 2024
@beutlich
Copy link
Member Author

PR contains only the required changes on the signal names. Please review again!

AHaumer
AHaumer approved these changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

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

Looks ok

@beutlich beutlich removed the request for review from henrikt-ma February 16, 2024 17:24
@beutlich beutlich merged commit f9bddf8 into modelica:master Feb 16, 2024
2 checks passed
@beutlich beutlich deleted the fix-signals branch February 16, 2024 17:24
@beutlich beutlich removed this from the MSL4.1.0 milestone Feb 16, 2024
This was referenced Feb 16, 2024
Merged
Open
@beutlich beutlich mentioned this pull request May 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HansOlsson HansOlsson HansOlsson approved these changes

@AHaumer AHaumer AHaumer approved these changes

Assignees
No one assigned
Labels
example Issue only addresses example(s) L: Resources Issue addresses Modelica/Resources (excl. C-Sources) V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@beutlich @henrikt-ma @HansOlsson @maltelenz @AHaumer