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

Fix #4312 #4522

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix #4312 #4522

wants to merge 2 commits into from

Conversation

HansOlsson
Copy link
Contributor

Avoid chattering in Modelica.Fluid.Examples.TraceSubstances.RoomCO2WithControls
This should correct the issue in #4312 while keeping the improvements.

It would be better if we could understand the original paper - a concern is that we are adding layer upon layers of extras.

@HansOlsson HansOlsson added the L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation) label Jan 21, 2025
@HansOlsson HansOlsson added this to the MSL4.1.0 milestone Jan 21, 2025
@HansOlsson HansOlsson requested a review from casella January 21, 2025 13:02
@HansOlsson
Copy link
Contributor Author

Closes #4312

@maltelenz
Copy link
Contributor

Using WSM, not only does this change not fix RoomCO2WithControls, it also breaks these (in either simulation or correctness checking) which work without the change:

Modelica.Fluid.Examples.AST_BatchPlant.BatchPlant_StandardWater
ModelicaTest.Fluid.TestComponents.Vessels.TestMixingVolumesPressureStates
ModelicaTest.Fluid.TestPipesAndValves.BranchingPipes16

I didn't dig closer (only ran our existing tests).

I would be interested in OpenModelica effects (or any other tool) for all Fluid-related tests with this change.

@HansOlsson HansOlsson mentioned this pull request Jan 21, 2025
@HansOlsson
Copy link
Contributor Author

Using WSM, not only does this change not fix RoomCO2WithControls, it also breaks these (in either simulation or correctness checking) which work without the change:

Modelica.Fluid.Examples.AST_BatchPlant.BatchPlant_StandardWater
ModelicaTest.Fluid.TestComponents.Vessels.TestMixingVolumesPressureStates
ModelicaTest.Fluid.TestPipesAndValves.BranchingPipes16

I didn't dig closer (only ran our existing tests).

I can also see differences in the results for these models (as indicated I didn't test it that completely).
So, instead of adding layer upon layer of work-arounds it may be that we should remove one of them - specifically the if-clause below:

      if abs(theta0 - c) < 1e-6 then
        // Slightly reduce c in order to avoid ill-posed problem
        c := (1 - 1e-6)*theta0;
      end if;
      rho := 3*(Delta0 - c)/(theta0 - c);

This seems to be causing the problems further down - in the original problem we had:

Delta=0.001
theta=0.00100053333
c=0.00099973333

and now we change c0 to 0.001005323 to allegedly avoid an ill-posed problem, causing rho to change from practically 1 to -1596.

Basically we are trying to increase the difference between c and theta0 - but end up decreasing it!

@HansOlsson
Copy link
Contributor Author

I changed to use a relative test, matching the fact that we do a relative decrease - and removed the other code.
(As it in some sense guarded against this bad intermediate result.)

As far as I could see RoomCO2WithControls, and the three regressions all run with correct results - and the original problem causing the change is also gone.

A potential down-side of using a relative test is that if c is zero it will never trigger, but:

  • Either theta0 is also zero and the code will cause a division by zero with and without this change (I believe previous tests avoid that).
  • or theta0 is not zero in which case using c:=0.999999*theta0; instead of zero would make it more - not less - ill-posed.

However, I don't know if this (updated) test is actually useful, but it seems less harmful.

@maltelenz
Copy link
Contributor

Again running tests without digging deeply, for WSM, the newest iteration:

  • Fixes RoomCO2WithControls
  • Fixes ModelicaTest.Fluid.TestComponents.Fittings.TestMultiPortTraceSubstances (we used to get results not matching the reference)
  • Makes ModelicaTest.Fluid.TestPipesAndValves.BranchingPipes18 go from a simulation failure because of event jittering, to a correctness failure

So looks more promising than the first round.

@HansOlsson HansOlsson requested a review from maltelenz January 21, 2025 15:01
@maltelenz
Copy link
Contributor

  • Makes ModelicaTest.Fluid.TestPipesAndValves.BranchingPipes18 go from a simulation failure because of event jittering, to a correctness failure

This was just a sampling issue, so this also went from simulation being stuck to completely fixed.

I don't know anything about the function being changed, nor do I have any experience in fluid modeling, but from the point of view of working tests in System Modeler, this change is great.

Let me know if you want me to leave an approving review, although it would be better if the library officers did that.

@beutlich beutlich changed the title Correct 4312 Fix #4312 Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modelica.Fluid.Examples.TraceSubstances.RoomCO2WithControls is problematic because of chattering
2 participants