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

Don't change lambda when no homotpy should be used #8109

Merged

Conversation

AnHeuermann
Copy link
Member

Related Issues

#8097

Purpose

Don't change the value of lambda if homotopy (for the non-linear loop) shouldn't be used.
Added a testcase from #8097 to testsuite.

@AnHeuermann AnHeuermann added this to the 1.19.0 milestone Nov 10, 2021
@AnHeuermann AnHeuermann self-assigned this Nov 10, 2021
Copy link
Contributor

@phannebohm phannebohm left a comment

Choose a reason for hiding this comment

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

LGTM. Nice and quick fix!

testsuite/simulation/modelica/initialization/homotopy6.mos Outdated Show resolved Hide resolved
@casella
Copy link
Contributor

casella commented Nov 10, 2021

@AnHeuermann, I see two failures in the tests.

Regarding PumpingSystem.mos, it concerns MSL 3.2, which is now obsolete. Never mind that. The PumpingSystem example works fine in MSL 4.0.0, where we applied several improvements for robustness, see MSL PR #3291, and there was no regression for that.

It is still broken in MSL 3.2.3, because some of these changes could not be applied on MSL 3.2.3. Again, never mind that, MSL 3.2.3 will be obsolete soon as well.

Regarding homotopy4.mos I'm not sure, but I suspect that the previously obtained result was wrong, because the intermediate steps were skipped, so maybe the nonlinear solver converged onto a different solution.

You can easily tell by first trying the test case with -ils=1 (i.e. one step from lambda = 0 straight to lambda = 1), and then gradually increasing the number of steps. If the solution is consistently different when a large enough number of steps is taken, I guess we could conclude that the previous results were wrong, and the current ones are fine.

@casella
Copy link
Contributor

casella commented Nov 10, 2021

@sjoelund, we need to get this PR through ASAP. It currently fails during the check-and-upload phase with

Failed to extract omc-clang.tar.gz

I'm not sure what it means, but I guess it is related to some of your most recent changes to the Jenkins infrastructure. Could you please check?

Thanks!

@casella
Copy link
Contributor

casella commented Nov 10, 2021

@matteodepascali, @janhaem96, we're almost done. Also check the master issue #8097,

@sjoelund
Copy link
Member

It's not related to changes to the infrastructure. It's related to a security patch in Jenkins

@casella
Copy link
Contributor

casella commented Nov 10, 2021

It's not related to changes to the infrastructure. It's related to a security patch in Jenkins

OK. I hope it can be fixed soon 😅

@AnHeuermann AnHeuermann enabled auto-merge (squash) November 11, 2021 08:39
@AnHeuermann AnHeuermann merged commit c32c675 into OpenModelica:master Nov 11, 2021
@AnHeuermann AnHeuermann deleted the homotopy-lambda-confusion branch November 11, 2021 10:45
@casella
Copy link
Contributor

casella commented Nov 11, 2021

Waiting for some resources to become available to compile a Windows nightly. Will get back to you ASAP.

@casella
Copy link
Contributor

casella commented Nov 12, 2021

@AnHeuermann, unfortunately the test report mixes up three different commits. I understand this commit could either cause an improvement Compile->Simulate or Compile->Verify, or a regression Simulate->Compile or Verify-Compile. There aren't many like that, most changes are due to #8019.

As I thought, there were not many models in the testsuite that really needed many step in the homotopy transformation. The two ThermoPower models seem to belong to that category.

@casella casella mentioned this pull request Nov 12, 2021
@AnHeuermann
Copy link
Member Author

I understand this commit could either cause an improvement Compile->Simulate or Compile->Verify, or a regression Simulate->Compile or Verify-Compile.

Yes, it should only affect the runtime.

The HeatingCoolingHotWater3Clusters example has some division by zero problems during initialization with homotopy. So this could be because of this commit.
My guess is that either the lambda=0.33 system is having some strange side effects or that some parameter/variable is 0 but shouldn't be.

I don't know why HeatingCoolingHotWaterSmall is failing now. It does look like a change introduced by #6195.

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.

None yet

4 participants