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

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.

https://doc.modelica.org

BSD 3-Clause "New" or "Revised" License

452 stars 165 forks source link

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

Closed beutlich closed 4 months ago

beutlich commented 4 months ago

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)~

henrikt-ma commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

beutlich commented 4 months ago

Hm, seems to be closed by accident.

beutlich commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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