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

Sync comparisonSignals.txt with MSL d2592ead8b #15

Open
wants to merge 2 commits into
base: v4.1.0
Choose a base branch
from

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Jan 24, 2025

Sync the comparisonSignals.txt with modelica/ModelicaStandardLibrary@d2592ea.

Note that MSL is seen as SSoT. I wonder why they differ at all.

@beutlich beutlich requested a review from GallLeo January 24, 2025 15:32
@maltelenz
Copy link
Contributor

From what I can tell, there are 4 categories of differences:

  1. Whitespace changes
  2. Reordering of the same variables
  3. The copy from ModelicaStandardLibrary has variables removed, but those variables still exist in this repo (both in comparisonSignals.txt and in the actual csv files.
  4. New files that don't have any references yet

I think 1 and 2 are unfortunate, and it could be helpful if LTX improved their tooling to not modify the comparisonSignals.txt they create, just taking the ones from ModelicaStandardLibrary.

3 is an effect of the decision to not update references, when variables are removed from the set to be tested, but no new variables are added. I'm not sure if we should do anything for this repository in this case?

4 is just that modelica/ModelicaStandardLibrary#4349 hasn't been finished yet, and introducing the comparisonSignals.txt for model we don't have reference results for yet seems like a bad idea.

In the end, I begin to question why this repo contains comparisonSignals.txt to begin with?

@beutlich
Copy link
Member Author

beutlich commented Jan 27, 2025

Thanks for the thorough wrap-up.

In the end, I begin to question why this repo contains comparisonSignals.txt to begin with?

When LTX or myself run the regression tests initially, it created all output files in the same folder where the comparisonSignals.txt are stored. So we copied the entire folder structure and created this repository including the comparisonSignals.txt (which got duplicated).

I'd expected the same approach for the subsequent regression test runs (like I setup for modelica/ModelicaStandardLibrary#4359 (comment)), hence I wonder why the comparisonSignals.txt ever can get out-of-sync.

@beutlich
Copy link
Member Author

@MatthiasBSchaefer @casella I've also added you as reviewer.

Copy link

@casella casella left a comment

Choose a reason for hiding this comment

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

I completely agree with @maltelenz. If the SSoT of comparison signals is in the MSL, we should just remove these file, rather than wasting time to keep them in synch.

@MatthiasBSchaefer when you run your scripts on the LTX server, you are taking the comparisonSignals.txt files from the MSL, not from this repo, right?

@casella
Copy link

casella commented Feb 14, 2025

As also noted in #17, once you have the CSV file, the information about what the reference signals are is in the first line of the CSV file, so that's one more reason to avoid keeping comparisonSignals.txt in this repo and wasting time with their maintenance.

@MatthiasBSchaefer
Copy link
Collaborator

@casella the nightly runs on our server are using the "correct" signalComparison.txt from https://github.com/modelica/ModelicaStandardLibrary

@beutlich
Copy link
Member Author

beutlich commented Feb 15, 2025

Other than trust to a closed-source regression-test-pipeline, it helps to have some more (machine-readable) reporting/checking which signals are actually taken in consideration.

In modelica-tools/msl-release@c454482 I have just introduced a result_passed.log logfile containing the considered variables. In case of failure the logfile result_failed.log is generated with an error message.

In that sense we can get rid of the comparisonSignals.txt files in this repo, provided we commit the result_passed.log or result_failed.log files per regression result.

the nightly runs on our server are using the "correct" signalComparison.txt from https://github.com/modelica/ModelicaStandardLibrary

If you mean comparisonSignals.txt, this makes me even more puzzled why these files could ever get out of sync.

@MatthiasBSchaefer
Copy link
Collaborator

MatthiasBSchaefer commented Feb 17, 2025

All information needed are in the set of files { comparisonSignals.txt , ModelName.csv, creation.txt, simulate_passed.log, translate_passed.log and check_passed.log}.
So i don't understand the necessity of any further file. Reading the first line of the csv file, shouldn't be an issue for "machine-readable reporting/checking"

Btw: The additional file "compare_passed.log" is - in my opinion - misplaced in this repo, especially since it is not clear what is compared there.

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.

4 participants