-
-
Notifications
You must be signed in to change notification settings - Fork 202
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
Update QNDF to new nlsolver #1165
Conversation
How's the latest benchmark? I assume that this would "fix it". |
After: |
@.. tmp = (uprev + tm - ϕ)/ (dt * β₀) | ||
|
||
else | ||
dz = nlsolver.cache.dz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am out of sync due to the MLH things, but we don't handle mass matrix intrinsically in nlsolve? tmp depending on mass matrix sounds spectical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright 👍 I need to see the new nlsolver changes.
|
||
@test _norm_dsol(QNDF1(),prob,prob2) ≈ 0 atol=1e-12 | ||
@test _norm_dsol(QNDF2(),prob,prob2) ≈ 0 atol=1e-12 | ||
@test _norm_dsol(QNDF(),prob,prob2) ≈ 0 atol=1e-7 | ||
@test _norm_dsol(QNDF(),prob,prob2) ≈ 0 atol=1e-12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is nice
I updated the plots slightly for better comparison (due to ode15s). |
nlsolver.z = uprev + sum(D) | ||
|
||
mass_matrix = f.mass_matrix | ||
|
||
if mass_matrix == I | ||
nlsolver.tmp = @.. (u₀ - ϕ)/ (dt * β₀) | ||
else | ||
_tmp = mass_matrix * @.. (u₀ - ϕ) | ||
nlsolver.tmp = @.. _tmp / (dt * β₀) | ||
end | ||
nlsolver.γ = β₀ | ||
nlsolver.α = α₀ | ||
nlsolver.method = COEFFICIENT_MULTISTEP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised about the mix of broadcasted and non-broadcasted expressions (not only in these lines, here it would be just uprev .+ sum(D)
, but also above e.g. in ϕ += γₖ[i] * D[i]
). Shouldn't we consistently use broadcasting? Or does it still impact performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, actually, there's some dimensional problem like that D is 1x5 matrix and γ is 6-Array, hence probably the person who did before looped using []; it was originally defined like that. We can change it probably for more consistent operation using broadcast, maybe in another PR.
Why are there two QNDFs in the plots? Is it one QNDF and one QBDF? Which one is better? The WPD curve looks really suspicious since the convergence looks erratic. |
The 'inverted green triangle' legend is QBDF and the other one is QNDF. |
Do you mind to benchmark |
Looks like it didn't move... which is odd. |
What's the benchmark problem? |
POLLU, with |
No description provided.