Rework FH problem: reduce allocations and improve efficiency#92
Rework FH problem: reduce allocations and improve efficiency#92MaxenceGollier wants to merge 10 commits intoJuliaSmoothOptimizers:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
===========================================
- Coverage 99.18% 67.29% -31.90%
===========================================
Files 11 19 +8
Lines 369 480 +111
===========================================
- Hits 366 323 -43
- Misses 3 157 +154 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
For example, with this version I am trying to get results with the older version but i am running in a ton of problems, it seems that the previous version was not even compatible with |
|
@MaxenceGollier this seems to be extraordinary, I will review it ASAP, yes sure we can meet! |
|
@MaxenceGollier Could you please check that objective, gradients, etc., match the old implementation? |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Fitzhugh–Nagumo (FH) problem implementation to reduce allocations and dependency weight by switching from DifferentialEquations to OrdinaryDiffEqVerner, and by using ReverseDiff-based AD backends.
Changes:
- Replace
DifferentialEquationsusage withOrdinaryDiffEqVernerand introduceReverseDifffor FH objective/residual evaluation. - Rework FH residual/objective to use in-place ODE solves with preallocated buffers.
- Update dependencies/compat and documentation to reflect the new required packages for FH.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test/runtests.jl |
Switch test deps to OrdinaryDiffEqVerner; SVM test blocks are currently wrapped in a triple-quoted string (disabled). |
src/RegularizedProblems.jl |
Gate FH model loading behind Requires on OrdinaryDiffEqVerner + ReverseDiff. |
src/fh_model.jl |
Major FH rewrite: integrator reuse, in-place residual, ReverseDiff backends. |
Project.toml |
Remove DifferentialEquations from test extras/targets; add OrdinaryDiffEqVerner + ReverseDiff. |
docs/src/index.md |
Document updated imports required for fh_model(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NLPModels, | ||
| NLPModelsModifiers, | ||
| OrdinaryDiffEqVerner, | ||
| QuadraticModels |
There was a problem hiding this comment.
fh_model is only included via @require ReverseDiff (see src/RegularizedProblems.jl), but ReverseDiff is not imported in this test file. As a result, fh_model() is likely undefined when the "FH" testset runs. Import ReverseDiff (or otherwise ensure it is Base.required) before calling fh_model() so the Requires hook fires.
test/runtests.jl
Outdated
| """ | ||
| @testset "SVM-Train" begin | ||
| ENV["DATADEPS_ALWAYS_ACCEPT"] = true |
There was a problem hiding this comment.
The triple-quoted string starting here turns the entire SVM testsets into a string literal, so these tests no longer run. If the goal is to avoid large dataset downloads on CI, consider guarding the testsets behind an env flag (and/or using a small local fixture) rather than silently disabling them.
| noise = 0.1 * randn(length(integrator.sol.u), 2) | ||
| noise = collect(eachrow(noise)) | ||
| data = noise + integrator.sol.u | ||
| temp = zeros(eltype(data[1]), length(data) * 2) |
There was a problem hiding this comment.
data = noise + integrator.sol.u will throw a MethodError in Julia because + is not defined for Vector/Vector containers (you likely want elementwise/broadcasted addition). Use broadcasting (e.g., noise .+ integrator.sol.u) or explicitly build data element-by-element so data[i] remains a 2-vector.
| function residual!(F, x :: AbstractVector{T}) where{T <: Real} | ||
| if integrator.p != x | ||
| OrdinaryDiffEqVerner.reinit!(integrator, u0) | ||
| integrator.p .= x | ||
| OrdinaryDiffEqVerner.solve!(integrator) |
There was a problem hiding this comment.
The cache check if integrator.p != x is not safe when callers reuse and mutate the same x vector between evaluations (common in optimization). In that case integrator.p can alias x, the comparison stays false, and the ODE solve is skipped even though parameters changed, returning a stale residual. Prefer always re-solving, or compare against a non-aliasing cached copy of the last parameters.
| function fh_model(; kwargs...) | ||
| data, simulate, resid, misfit, x0 = FH_smooth_term() | ||
| data, resid!, misfit, x0 = FH_smooth_term() | ||
| nequ = 202 | ||
| ADNLPModels.ADNLPModel(misfit, ones(5); kwargs...), | ||
| ADNLPModels.ADNLSModel(resid, ones(5), nequ; kwargs...), | ||
| x0 | ||
| end | ||
| nlp = ADNLPModels.ADNLPModel(misfit, ones(5); | ||
| hessian_backend = ADNLPModels.ReverseDiffADHessian, |
There was a problem hiding this comment.
fh_model(; kwargs...) no longer forwards kwargs... to ADNLPModel/ADNLSModel!, even though the docstring says all keyword arguments are passed through (and callers like setup_fh_l0/l1 forward kwargs). This is a breaking API change and prevents users from overriding AD backends/options. Consider keeping the ReverseDiff backends as defaults but still merging/forwarding kwargs... so callers can customize behavior.
|
|
|
|
@dpo, @MohamedLaghdafHABIBOULLAH
closes #91
partially closes #90 by adding the OrdinaryDiffEqVerner dep instead of the much much much larger DifferentialEquations dep.
I will use this for the last problem of #88, with
NLPModelsModifiers.jl.I removed the MLDataSet from the dep because I had compat issues. I am waiting for chengchingwen/Pickle.jl#47 to add it back.
I think nonetheless that having to download a large dataset on each CI run is not a good idea and should be avoided (50 minutes of CI for a package of this scale is ridiculous).
@MohamedLaghdafHABIBOULLAH, this is breaking i would like to see your review on this one and make sure that this does not destroy to much code. Can we meet at GERAD sometime ?