ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

MSL 4.1.0 Regressions - Media

Open GallLeo opened this issue 1 year ago • 3 comments

he following models fail in result comparison. Tested revision: f9bddf86 (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 #4430. - Updated reference files?
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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
  • [x] 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 e4b68715259b1ce44bff8e4050908f06cee5a9b0 to restore removed models which was then fixed by @hubertus65 in #3918. - Updated reference files? @GallLeo plese generate new reference files
  • [x] 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 e4b68715259b1ce44bff8e4050908f06cee5a9b0 to restore removed models which was then fixed by @hubertus65 in #3918. - Updated reference files? @GallLeo plese generate new reference files
  • [x] 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 e4b68715259b1ce44bff8e4050908f06cee5a9b0 to restore removed models which was then fixed by @hubertus65 in #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 #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 avatar Feb 27 '24 08:02 GallLeo

  • ModelicaTest.Media.TestAllProperties.MoistAir

I run git bisect on that test model and it reported

cc408ce564d2af08d76a416b434a63bcae61e79e 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 avatar Mar 23 '24 14:03 beutlich

  • ModelicaTest.Media.TestOnly.R134a_setState_pTX

I run git bisect on that test model and it reported

f1574e8116fd5e4d72c5cbae4176f29e1dc3445f 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.

beutlich avatar Mar 23 '24 14:03 beutlich

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

hubertus65 avatar Jun 11 '24 20:06 hubertus65

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

hubertus65 avatar Nov 11 '24 14:11 hubertus65

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 avatar Nov 11 '24 14:11 maltelenz

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

casella avatar Nov 11 '24 16:11 casella

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

maltelenz avatar Nov 11 '24 16:11 maltelenz

Sorry, I was looking at the wrong Essotherm650 😅

casella avatar Nov 11 '24 17:11 casella

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?

maltelenz avatar Jan 28 '25 13:01 maltelenz

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 https://github.com/modelica/MAP-LIB_ReferenceResults/commit/af2c8f2fe0720044d593d560db635321300afe1b being the latest change as of today.

beutlich avatar Jan 28 '25 17:01 beutlich

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

maltelenz avatar Feb 11 '25 12:02 maltelenz

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!

casella avatar Feb 11 '25 15:02 casella

All issues settled.

casella avatar Feb 27 '25 11:02 casella