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

Newmark beta method #2187

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

termi-official
Copy link
Contributor

Prototype to resolve #411 .

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

src/alg_utils.jl Outdated
@@ -495,6 +495,7 @@ alg_order(alg::ERKN4) = 4
alg_order(alg::ERKN5) = 5
alg_order(alg::ERKN7) = 7
alg_order(alg::RKN4) = 4
alg_order(alg::NewmarkBeta) = 1 # or at least 2, depending on the parameters.
Copy link
Member

Choose a reason for hiding this comment

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

Since the coefficients are part of the algorithm, can you calculate the order here?

Copy link
Contributor

Choose a reason for hiding this comment

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

similar to FBDF, you can just set this to 1 I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look Hendrik! Yes, I could compute them here. I will fix such stuff after I get a pendulum example working. I just opened the PR to discuss the progress and possibly better design choices for the new class of solvers.

@termi-official
Copy link
Contributor Author

termi-official commented May 15, 2024

The last remaining problem is a bit tricky. For a given DynamicalODEFunction Newmark needs to solve for a "future accelleration" by solving the problem $Δtₙ f₁(innertmp₂ + z β Δtₙ², innertmp₁ + z γ Δtₙ,t) = z$ for $z$ with given $innertmp, \alpha,\beta$.

The first issue I run into is passing down the analytical Jacobian:

using OrdinaryDiffEq
function f1_harmonic(dv, v, u, p, t)
    dv .= -u
end
function f2_harmonic(du, v, u, p, t)
    du .= v
end
harmonic_jac_f1(J, v, u, p, t) = J[1,1] = -1.0
ff_harmonic = DynamicalODEFunction(f1_harmonic, f2_harmonic; jac = harmonic_jac_f1)
DiffEqBase.has_jac(ff_harmonic) # false
DiffEqBase.has_jac(ff_harmonic.f1) # false
ff_f1 = ODEFunction{false}(ODEFunction{false}(f1_harmonic); jac=harmonic_jac_f1!)
ff_harmonic = DynamicalODEFunction(ff_f1, f2_harmonic; jac = harmonic_jac_f1)
DiffEqBase.has_jac(ff_harmonic) # false
DiffEqBase.has_jac(ff_harmonic.f1) # false 

The second problem is getting the AD on track. My current strategy is to pass a helper type ArrayPartitionNLSolveHelper around, which constructs an ArrayPartition when multiplied with a Vector. The idea is to provide an interface for inplace evaluation of f.f1 (DynamicalODEFunction) which is compatible with the evaluation form required by the internal nonlinear solver, such that we can call $f₁(innertmp + z \alpha,t)$, where $\alpha$ is the mentioned ArrayPartitionNLSolveHelper. Should we head down this road or is there possibly a better design?

@termi-official
Copy link
Contributor Author

I think I can call this now a first prototype. I am not very sure why it manages to converge to the correct solution, but I assume the residual is just wrong up to scaling. Another problem is that the Jacobian is per construction wrong. The derivative of $f_1$ w.r.t. the input parameter is a rectangular matrix. For Newmark we assume that the input space is partitioned into 2 equally sized parts (velocity $v$ and displacement $u$) and that $f_2$ is just $du = v$. The first assumption tells us that the Jacobian is blocked with 2 blocks. For the inner nonlinear solve in Newmark we now obtain the system matrix for the Newton as $\partial_z f_1(u+\gamma_1 z,v+\gamma_2 z) = \gamma_1 J_{1,1}(u+\gamma_1 z,v+\gamma_2 z) + \gamma_2 J_{1,2}(u+\gamma_1 z,v+\gamma_2 z)$ where $J_{i,j}$ is the i,j-th block of the Jacobian of $f$.

However, this seems to break a few assumptions taken in the code. Hence I could need some feedback on how to proceed before putting time into a design which we do not want in the library. I already think that ArrayPartitionNLSolveHelper is not great, but I cannot figure out a better design by myself either.

Furthermore I cannot figure out how to integrate the W-transformation here. Any pointers?

@@ -507,6 +507,7 @@ nlsolve_f(f, alg::DAEAlgorithm) = f
function nlsolve_f(integrator::ODEIntegrator)
nlsolve_f(integrator.f, unwrap_alg(integrator, true))
end
nlsolve_f(f::DynamicalODEFunction, alg::NewmarkBeta) = f.f1 # FIXME
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a very hard time figuring out whether this seems obviously fine, or an incredibly bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, as you pointed out on Slack we should switch to SecondOrderODEFunction IIUC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay the reason why I did this was that there is not SecondOrderODEFunction. Can we easily infer from the DynamicalODEFunction that the second function is only f2(du,u,v,p,t) = v?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, that's weird. we have a SecondOrderODEProblem but no SecondOrderODEFunction.

src/misc_utils.jl Outdated Show resolved Hide resolved
@@ -303,7 +303,7 @@ end
# page 54.
γdt = isdae ? α * invγdt : γ * dt

!(W_γdt ≈ γdt) && (rmul!(dz, 2 / (1 + γdt / W_γdt)))
# !(W_γdt ≈ γdt) && (rmul!(dz, 2 / (1 + γdt / W_γdt)))
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this random line commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not get it to work with Newmark, because here we have a comparison between a scalar W_γdt and a pair γdt.

function _compute_rhs!(tmp, ztmp, ustep, γ::ArrayPartitionNLSolveHelper, α, tstep, k,
invγdt, method::MethodType, p, dt, f, z)
mass_matrix = f.mass_matrix
ustep = tmp + γ * z
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't this compute_ustep!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to double check what exactly lead to this change, but I think we do not need this anymore.

Comment on lines 92 to 96
mutable struct NLSolver{algType, iip, uType, gamType, tmpType, tmp2Type, tType,
C <: AbstractNLSolverCache} <: AbstractNLSolver{algType, iip}
z::uType
tmp::uType # DIRK and multistep methods only use tmp
tmp2::tmpType # for GLM if neccssary
tmp::tmpType # DIRK and multistep methods only use tmp
tmp2::tmp2Type # for GLM if neccssary
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're going to do this, I think it's probably worth refactoring to use innertmp and outertmp for consistency (and then we can unify the handling by setting the appropriate one to 0 for the method being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should go into a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

integrator.f(cache.fsalfirst, integrator.uprev, integrator.p, integrator.t)
integrator.stats.nf += 1
integrator.fsalfirst = cache.fsalfirst
integrator.fsallast = cache.fsalfirst
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you ask like this, then I think I am understanding something wrong here. From looking at other FSAL methods I thought that we can just put the same memory into both locations.

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind on this one then. I haven't read through other ones in a while, so this looked like a copy paste error, but if other methods do it also, then I think this is fine.

src/perform_step/newmark_perform_step.jl Outdated Show resolved Hide resolved
Comment on lines 30 to 33
upred_full = ArrayPartition(
duprev + dt*(1.0 - γ)*dduprev,
uprev + dt*dt*(0.5 - β)*dduprev + dt*duprev
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like it will allocate a bunch, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. As also pointed out below by you during review we might be able to use nlsolver.tmp to get rid of this.

# Note: innertmp = nlsolve.tmp
nlsolver.γ = ArrayPartitionNLSolveHelper(γ, β * dt) # = γ̂
# nlsolver.γ = ArrayPartitionNLSolveHelper(0.0, β * dt)
nlsolver.tmp .= upred_full # TODO check f tmp is potentially modified and if not elimiate the allocation of upred_full
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe nlsolver.tmp isn't modified, but I'm not 100% sure.

# dt⋅f(innertmp + γ̂⋅z, p, t + c⋅dt) + outertmp = z
# So we rewrite the problem
# u(tₙ₊₁)'' - f₁(ũ(tₙ₊₁) + u(tₙ₊₁)'' β Δtₙ², ũ(tₙ₊₁)' + u(tₙ₊₁)'' γ Δtₙ,t) = 0
# z = Δtₙ u(tₙ₊₁)'':
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we generally put the Δtₙ as part of γ rather than z. Is there a reason not to do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the $\gamma$ here is the one from the Newmark solver while $\hat{\gamma}$ is the one from the Newton solver.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh. that's awful.

Copy link
Contributor Author

@termi-official termi-official left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback. I have updated the implementation to master and made the tests for linear problems (harmonic and damped oscillator) pass. However, I had to manually unroll the implementation. I left some inline comments here on Github on the code sections where I am stuck (and on which I will resume working after fixing the most pressing issues).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will strip down the imports to the used ones after we have fixed the remaining parts.

@@ -0,0 +1,5 @@
alg_extrapolates(alg::NewmarkBeta) = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will recover the extrapolation once we have ironed out the remaining parts.

Comment on lines +69 to +82
uₙ₊₁ = uₙ + dt * vₙ + dt^2/2 * ((1-2β)*aₙ + 2β*aₙ₊₁)
vₙ₊₁ = vₙ + dt * ((1-γ)*aₙ + γ*aₙ₊₁)
# Compute residual
f.f1(atmp, vₙ₊₁, uₙ₊₁, p, t)
integrator.stats.nf += 1
residual = M*(aₙ₊₁ - atmp)
# Compute jacobian
f.jac(J, vₙ₊₁, uₙ₊₁, (γ*dt, β*dt*dt), p, t)
# Solve for increment
Δaₙ₊₁ = (M-J) \ residual
aₙ₊₁ .-= Δaₙ₊₁ # Looks like I messed up the signs somewhere :')
increment_norm = integrator.opts.internalnorm(Δaₙ₊₁, t)
increment_norm < 1e-4 && break
i == 10 && error("Newton diverged. ||Δaₙ₊₁||=$increment_norm")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oscardssmith I think I need some help here regarding the integration with OrdinaryDiffEqNonlinearSolve.jl .

A critical missing piece is an interface for the evaluation of the Jacobian in a suitable form (especially via ad). For now I have just bypassed this with a custom jacobian function, e.g. for the harmonic oscillator in the test

    function harmonic_jac(J, v, u, weights, p, t)
        J[1,1] = weights[1] * (0.0) + weights[2] * (-1.0)
        J[1,2] = weights[1] * (0.0) + weights[2] * ( 0.0)
        J[2,2] = weights[1] * (0.0) + weights[2] * (-1.0)
        J[2,1] = weights[1] * (0.0) + weights[2] * ( 0.0)
    end

because many implicit second order ODE methods requires that J = Δtₙ²β ∂fᵤ + Δtₙγ ∂fᵥ . See above for a "full" derivation.

Comment on lines +16 to +18
function f2_harmonic!(du, v, u, p, t)
du .= v
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the intended way to enforce this automatically? We do not have a SecondOrderODEFunction analogue to DynamicalODEFunction.

src/misc_utils.jl Outdated Show resolved Hide resolved
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.

Numark-beta method
3 participants