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 4.1.0 Regressions - Media #4339

Open
17 tasks done
GallLeo opened this issue Feb 27, 2024 · 12 comments
Open
17 tasks done

MSL 4.1.0 Regressions - Media #4339

GallLeo opened this issue Feb 27, 2024 · 12 comments
Assignees
@casella @hubertus65 @arunkumar-narasimhan
Labels
L: Media Issue addresses Modelica.Media L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Milestone
MSL4.1.0

Comments

@GallLeo
Copy link
Contributor

GallLeo commented Feb 27, 2024 •
edited by hubertus65
Loading

he following models fail in result comparison.
Tested revision: f9bddf8 (2024-02-16)

Changed models, need reference update after library officer check:

  • ModelicaTest.Media.TestAllProperties.MoistAir
    - Reason: err_h_is is included in the reference file, but it shouldn't, it makes no sense to do regression on relative errors, they are just meant for the assert statments. Action: remove err_T, err_d, err_u, err_h_is from the reference.txt file, see PR Remove error signals from MoistAir Tests #4430.
    - Updated reference files?
  • ModelicaTest.Media.TestOnly.R134a_setState_pTX
    - Reason: the reference file is badly wrong at time = 0.4014, all variables in the reference file have bogus values like -1.8e98. Simulation of the model with Dymola 2024 x and OMC 1.24.0-dev.123 gives no such bogus values. Action: re-generate the files, but then quickly check that the values far from 0.401 are still the same.
    - Updated reference files? @GallLeo please generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Air.DryAirNasa
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Air.MoistAir
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Air.SimpleAir
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.IdealGases.Air
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.IdealGases.Nitrogen
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.IdealGases.SimpleNaturalGas
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.IdealGases.SimpleNaturalGasFixedComposition
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.LinearFluid.LinearColdWater
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.LinearFluid.LinearWater_pT
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Water.IdealSteam
    - Reason: bogus pressure values at 1000101325 Pa, should be 1101325. Not sure how these values managed to survive for so long. Action: re-generate the result files
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Water.WaterIF97OnePhase_ph
    - Reason: pipe model in 4.0.0 had dp_nom = 1e9, resulting in very high hydraulic resistance of the pipe (which makes non sense). 4.1.0 has 1e6, which is OK. The change seems reasonable, but we need to figure out when and where it happened to close the issue. Started from this commit e4b6871 to restore removed models which was then fixed by @hubertus65 in Update ModelicaTest/Media.mo: Reduce dp_nominal in TestsWithFluid.Components.PartialTestModel by a factor of 1000 #3918.
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Water.WaterIF97_ph
    - Reason: pipe model in 4.0.0 had dp_nom = 1e9, resulting in very high hydraulic resistance of the pipe (which makes non sense). 4.1.0 has 1e6, which is OK. The change seems reasonable, but we need to figure out when and where it happened to close the issue. Started from this commit e4b6871 to restore removed models which was then fixed by @hubertus65 in Update ModelicaTest/Media.mo: Reduce dp_nominal in TestsWithFluid.Components.PartialTestModel by a factor of 1000 #3918.
    - Updated reference files? @GallLeo plese generate new reference files
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Water.WaterIF97_pT
    - Reason: pipe model in 4.0.0 had dp_nom = 1e9, resulting in very high hydraulic resistance of the pipe (which makes non sense). 4.1.0 has 1e6, which is OK. The change seems reasonable, but we need to figure out when and where it happened to close the issue. Started from this commit e4b6871 to restore removed models which was then fixed by @hubertus65 in Update ModelicaTest/Media.mo: Reduce dp_nominal in TestsWithFluid.Components.PartialTestModel by a factor of 1000 #3918.
    - Updated reference files? @GallLeo plese generate new reference files

Change due to new Dymola version (reference update after library officer check?):

  • ModelicaTest.Media.TestAllProperties.IncompleteMedia.ReferenceMoistAir
    - Reason: Verification failures with err_T, err_d, err_u. Signals like err_T is included in the reference file, but it shouldn't, it makes no sense to do regression on relative errors, they are just meant for the assert statments. Action: remove err_T, err_d, err_u, err_h_is from the reference.txt file, see PR Remove error signals from MoistAir Tests #4430.
    - Updated reference files?

  • Release notes check: Are all classes mentioned which could lead to result changes in user models?

Useful Links

Current comparison report:
https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/comparison_report_overview.html
-> Reference result test -> Comparison

Comparison signal definitions:
https://github.com/modelica/ModelicaStandardLibrary/tree/master/Modelica/Resources/Reference/Modelica
https://github.com/modelica/ModelicaStandardLibrary/tree/master/ModelicaTest/Resources/Reference/ModelicaTest

Reference results:
https://github.com/modelica/MAP-LIB_ReferenceResults
@GallLeo GallLeo added L: Media Issue addresses Modelica.Media L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases) labels Feb 27, 2024
@GallLeo GallLeo added this to the MSL4.1.0 milestone Feb 27, 2024
@beutlich
Copy link
Member

beutlich commented Mar 23, 2024 •
edited
Loading

  • ModelicaTest.Media.TestAllProperties.MoistAir

I run git bisect on that test model and it reported

cc408ce is the first bad commit

The error is only for signal err_h_is now being exactly 0 (while being 1.8189894035458565e-12 before). See #4238.

@beutlich
Copy link
Member

beutlich commented Mar 23, 2024 •
edited
Loading

  • ModelicaTest.Media.TestOnly.R134a_setState_pTX

I run git bisect on that test model and it reported

f1574e8 is the first bad commit

#3704 is an actual bug fix that I already back-ported to branches maint/3.2.3 (#3790) and maint/4.0.x (#3791) about 3 years ago.

@hubertus65
Copy link
Member

I had a quick look at these, and all of them look to me like "valid" changes: below numerical accuracy, or based on fixes to the test models

@casella casella self-assigned this Jun 22, 2024
GallLeo added a commit to modelica/MAP-LIB_ReferenceResults that referenced this issue Oct 20, 2024
@GallLeo
Update ModelicaTest.Media
60b9434 
as defined modelica/ModelicaStandardLibrary/issues/4339
@hubertus65
Copy link
Member

As far as I can see, the issue should be closed, all done

@maltelenz
Copy link
Contributor

There seem to be correctness errors in the report for these?

  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Incompressible.Essotherm650
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Incompressible.Glycol47
  • ModelicaTest.Media.TestsWithFluid.MediaTestModels.Water.ConstantPropertyLiquidWater

Reopening for clarification.

@maltelenz maltelenz reopened this Nov 11, 2024
@casella
Copy link
Contributor

casella commented Nov 11, 2024

@maltelenz in the report you cite I see all green for those three models. Which errors are you referring to?

@maltelenz
Copy link
Contributor

image
This is what I see. The CSVCompare (last column) is red.

@casella
Copy link
Contributor

casella commented Nov 11, 2024

Sorry, I was looking at the wrong Essotherm650 😅

beutlich pushed a commit to modelica/MAP-LIB_ReferenceResults that referenced this issue Jan 21, 2025
@GallLeo @beutlich
Update ModelicaTest.Media
92c4996 
as defined modelica/ModelicaStandardLibrary/issues/4339
@maltelenz
Copy link
Contributor

As warned about in #4238 , I am seeing (small) regressions for ModelicaTest.Media.TestAllProperties.MoistAir when testing with WSM:

a
reference: 347.024351711   trial: 347.025144721   delta = 7.930095E-04

cp
reference: 1013.31888176   trial: 1013.32351297   delta = 4.631216E-03

cp2
reference: 1013.31888176   trial: 1013.32351297   delta = 4.631216E-03

cv
reference: 724.522935763   trial: 724.526247082   delta = 3.311319E-03

cv2
reference: 724.522935763   trial: 724.526247082   delta = 3.311319E-03

h
reference: 50329.2238194   trial: 50328.5907185   delta = 6.331009E-01

h2
reference: 50329.2238194   trial: 50328.5907185   delta = 6.331009E-01

h_is
reference: 116306.96664   trial: 116306.302865   delta = 6.637751E-01

s
reference: 6924.01316181   trial: 6924.04469876   delta = 3.153695E-02

s_is
reference: 6924.01316181   trial: 6924.04469876   delta = 3.153695E-02

I confirmed that reverting #4238 "fixed" the regression.

I propose that we ask LTX to replace the reference result. @casella, do you agree?

@beutlich
Copy link
Member

Confirmed, ModelicaTest.Media.TestAllProperties.MoistAir is not yet updated in the v4.1.0 branch of the ref result, see https://github.com/modelica/MAP-LIB_ReferenceResults/tree/v4.1.0/ModelicaTest/Media/TestAllProperties/MoistAir with modelica/MAP-LIB_ReferenceResults@af2c8f2 being the latest change as of today.

@maltelenz
Copy link
Contributor

@casella this is waiting for your decision, see my previous comment.

@casella
Copy link
Contributor

casella commented Feb 11, 2025

I propose that we ask LTX to replace the reference result. @casella, do you agree?

Sound good to me; even though the difference is neglible for all practical purposes, there is indeed no point at keeping the old one.

@GallLeo, @MatthiasBSchaefer can you please take care of that? Thanks!

MatthiasBSchaefer added a commit to MatthiasBSchaefer/MAP-LIB_ReferenceResults that referenced this issue Feb 12, 2025
@MatthiasBSchaefer
Update Reference for ModelicaTest.Media.TestAllProperties.MoistAir ac…
54a3c02 
…cording to modelica/ModelicaStandardLibrary#4339
@MatthiasBSchaefer MatthiasBSchaefer mentioned this issue Feb 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@casella casella

@hubertus65 hubertus65

@arunkumar-narasimhan arunkumar-narasimhan

Labels
L: Media Issue addresses Modelica.Media L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Projects
None yet
Milestone
MSL4.1.0
Development

No branches or pull requests
6 participants
@maltelenz @casella @beutlich @hubertus65 @GallLeo @arunkumar-narasimhan