open-ideas / IDEAS

Modelica library allowing simultaneous transient simulation of thermal and electrical systems at both building and feeder level.
131 stars 56 forks source link

IDEAS.Examples.Tutorial.Example9 unit test takes too long #1336

Closed jelgerjansen closed 11 months ago

jelgerjansen commented 11 months ago

IDEAS.Examples.Tutorial.Example9 takes about 50 minutes to execute on the branch project_itz_fixes (#1327). We know that this model is the reason for the high execution time of IDEAS.Examples.Tutorial because it was the only model with an expired time out in this unit test: https://github.com/open-ideas/IDEAS/actions/runs/6615637929/job/17968232815.

In the master branch, this particular unit test finishes in about 5 minutes, so the changes in #1322 and #1327 might have caused this. However, when I run the 'simulate and plot' command on my local Dymola (2022x version), the simulation times for both branches are similar, but the evolution of the CPU time over the simulation time is different. This shows that the previously mentioned PRs does not directly affect the total simulation time (which would make sense, because no drastic changes were implemented).

@JavierArroyoBastida is this something you can have a look at? Adding the CPU time to the outputs might already give a clue whether the problem lies in the translation or simulation of Example9 on the server.

javiarrobas commented 11 months ago

@jelgerjansen Ok, I'll have a look at it. Hopefully by tomorrow I can spare some time. A couple of questions:

jelgerjansen commented 11 months ago

@JavierArroyoBastida there is no rush. If you want, we can also have a look at this together or if you don't have time I can also look at this myself.

javiarrobas commented 11 months ago

Ok, very strange indeed! No worries, I'll have a look at it. Hopefully I can figure it out, otherwise may help to look at it together. I'll let you know how it goes.

javiarrobas commented 11 months ago

Running in Dymola GUI in Windows takes around 3 minutes and works but throws the following errors:

image

However, I also get that running that same example in our testing Docker environment in Linux stalls at some point, even though we're using the same Dymola version (2022x). It seems that it fails in simulation because I see that dymosim starts but at some point it's stuck with the exact same memory use:

image

My conclusion is that it's a very specific corner case where the implemented edits affect the simulation in the Dymola Linux installation for some reason.

Increasing the tolerance from tolerance=1e-6 to tolerance=1e-5 fixes the issue and has the test simulating in the Dymola Linux environment as well. This change affects reference results as follows:

image

@jelgerjansen @Mathadon if you agree I'll update tolerance to be tolerance=1e-5 and update reference results accordingly.

jelgerjansen commented 11 months ago

@JavierArroyoBastida I think you mean going from a tolerance of 1e-4 to 1e-5? I see that all other examples (1-8) use a tolerance of 1e-6. Using that value (1e-6) for Example 9 stalls the simulation in my own Dymola installation. The original value of 1e-4 was therefore chosen on purpose I think.

I therefore agree changing the tolerance to 1e-5 instead of 1e-4. This speeds up the simulation somehow and will lead to more correct simulation results.

javiarrobas commented 11 months ago

Yes, sorry I was testing a different example before, so you can omit the last part of my comment above. However, it is still the tolerance what explains the difference between running the example in the Dymola GUI and the Dymola within the testing environment. The issue is that the model defines a tolerance of 0.00011 (see here) whereas the test overwrites such tolerance to be 1e-4 see here. When changing the tolerance of the test to be the one of the model I could run the test in 348.004 s without having to change reference results. I'll send a PR against https://github.com/open-ideas/IDEAS/pull/1327.

javiarrobas commented 11 months ago

PR sent: https://github.com/Mathadon/IDEAS/pull/1.

Mathadon commented 11 months ago

awesome! merged :)

jelgerjansen commented 11 months ago

@JavierArroyoBastida could you also merge this in the master branch? If that doesn't introduce problems, we can close this issue.

javiarrobas commented 11 months ago

That edit will be introduced when merging https://github.com/open-ideas/IDEAS/pull/1327, I see no need for an extra PR right?

jelgerjansen commented 11 months ago

Merging that PR might still take some time, but the unit tests in the master branch don't have this long simulation time problem, so this is okay for now. I'll close the issue then.