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

Gitlab CI does not include unit tests for OrdinaryDiffEqNonlinearSolve #2572

Closed
lte678 opened this issue Jan 10, 2025 · 2 comments
Closed

Gitlab CI does not include unit tests for OrdinaryDiffEqNonlinearSolve #2572

lte678 opened this issue Jan 10, 2025 · 2 comments
Labels

Comments

@lte678
Copy link
Contributor

lte678 commented Jan 10, 2025

It seems to me that the tests for the subpackage OrdinaryDiffEqNonlinearSolve were never added to the gitlab CI after splitting it off into its own package (commit c399c19 perhaps).

I fixed it up, but the currently existing newton_tests.jl unfortunately don't pass with the following error message:

Newton Tests: Test Failed at /home/lteichro/Projects/scratchpad/ordinarydiffeq_bug1214/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqNonlinearSolve/test/newton_tests.jl:17
  Expression: sol2.stats.nf <= sol1.stats.nf
   Evaluated: 102023 <= 102008

For the following assertion:

for prob in (prob_ode_lorenz, prob_ode_orego)
    sol1 = solve(prob, Trapezoid(), reltol = 1e-12, abstol = 1e-12)
    @test sol1.retcode == DiffEqBase.ReturnCode.Success
    sol2 = solve(prob, Trapezoid(nlsolve = NLNewton(relax = BackTracking())),
        reltol = 1e-12, abstol = 1e-12)
    @test sol2.retcode == DiffEqBase.ReturnCode.Success
    @test sol2.stats.nf <= sol1.stats.nf
end

Is it ok to toss the test or the test case? It seems like a quite mundane failure to me, unless there a reason to expect a strictly lesser amount of function evaluations.

I would like to contribute an unrelated fix to that OrdinaryDiffEqNonlinearSolve , for which I would like to get unit tests running again.

@lte678 lte678 added the bug label Jan 10, 2025
@ChrisRackauckas
Copy link
Member

Oh no, that's a bad omission. Thanks for catching this.

I don't think it's an issue for backtracking to take slightly more steps than the other method. There's nothing theoretical backing that assumption, seems to just be a regression test but with the norm change that could definitely just be a random thing that it occurred in the other direction before. I would just add @test sol2.stats.nf+20 <= sol1.stats.nf and that's a sufficient update to the test.

@lte678
Copy link
Contributor Author

lte678 commented Jan 22, 2025

Fixed in #2573
Tests are passing.

@lte678 lte678 closed this as completed Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants