JuliaSmoothOptimizers / SolverTools.jl

Tools for developing nonlinear optimization solvers.
Other
26 stars 18 forks source link

Added `ARTrustRegion` #210

Closed Jay-sanjay closed 2 months ago

Jay-sanjay commented 6 months ago

This PR is related to - https://github.com/JuliaSmoothOptimizers/AdaptiveRegularization.jl/issues/92

closes - https://github.com/JuliaSmoothOptimizers/AdaptiveRegularization.jl/issues/92

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 94.44444% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.51%. Comparing base (c1f7fbb) to head (46b442b). Report is 1 commits behind head on main.

Files Patch % Lines
src/trust-region/ar-trust-region.jl 94.44% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #210 +/- ## ========================================== + Coverage 97.32% 97.51% +0.18% ========================================== Files 8 9 +1 Lines 224 241 +17 ========================================== + Hits 218 235 +17 Misses 6 6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 6 months ago
Package name latest stable
DCISolver.jl
JSOSolvers.jl
Percival.jl
Jay-sanjay commented 5 months ago

HI @tmigot . Sir I was a bit confused with how the tests must look like. Should they just check what all things it is returning like the below once or something else is required.

  @testset "ARTrustRegion" begin
    Δ₀ = 0.8
    ar = ARTrustRegion(0.8)
    @test ar.α₀ == 0.8 
    @test ar.α == 0.8
    @test ar.max_α ≈ 1 / sqrt(eps(typeof(0.8)))
    @test ar.acceptance_threshold == 0.1
    @test ar.increase_threshold == 0.75
    @test ar.reduce_threshold == 0.1
    @test ar.increase_factor == 5.0
    @test ar.decrease_factor == 0.1
    @test ar.large_decrease_factor == 0.01
    @test ar.max_unsuccinarow == 30
  end

And also to just let you know, I have also asked you the same thing on slack, there we can take on a conversation for this :) thanks !

tmigot commented 5 months ago

I replied on Slack. Unit tests should check the constructors (as you suggested) but also add check for the added function.

github-actions[bot] commented 2 months ago
Package name latest stable
DCISolver.jl
JSOSolvers.jl
Percival.jl
github-actions[bot] commented 2 months ago
Package name latest stable
DCISolver.jl
JSOSolvers.jl
Percival.jl
Jay-sanjay commented 2 months ago

Thanks for the review @tmigot , also I was a bit curious that how come codecov passed all checks by adding extra test-cases (like 2 more extra testcases). Did they covered any edge cases ? Or what else ?

tmigot commented 2 months ago

Yes, it was covering edge cases in the compute_r function