SciML / Optimization.jl

Mathematical Optimization in Julia. Local, global, gradient-based and derivative-free. Linear, Quadratic, Convex, Mixed-Integer, and Nonlinear Optimization in one simple, fast, and differentiable interface.
https://docs.sciml.ai/Optimization/stable/
MIT License
688 stars 75 forks source link

fixed user `trace!` overload compat. Both `x` and user records now saved #702

Closed jonathanfischer97 closed 4 months ago

jonathanfischer97 commented 5 months ago

Checklist

Additional context

  1. This PR improves compatibility with Evolutionary.jl trace! functionality. Specifically, Evolutionary.jl allows user-defined overloads of trace! in order to save other values during optimization, via an generic interface. This functionality is advertised within the Evolutionary.jl docs, seen here:

    image
  2. When using OptimizationEvolutionary however, this functionality was broken. Previously, any user overloads of trace! would be dispatched in-place of the existing overload in OptimizationEvolutionary, allowing the possibility of "x" not being saved and thus throwing an error during construction of OptimizationState.

    # Previous overload in order to make sure `population` was saved
    function Evolutionary.trace!(record::Dict{String, Any}, objfun, state, population,
        method::Evolutionary.AbstractOptimizer, options)
    record["x"] = population
    end
  3. To fix this, I've instead replaced the previous overload with an overload of the internal Evolutionary.trace! function, which calls the exposed trace! function. This overload adds "x" to the existing "time" key which will always be saved, prior to calling user methods and regardless of their existence.

    # Overload the trace! function to add the population to the trace prior to calling any user-defined trace! method
    function Evolutionary.trace!(tr, iteration, objfun, state, population, method::Evolutionary.AbstractOptimizer, options, curr_time=time()) 
    dt = Dict{String,Any}()
    dt["time"] = curr_time
    
    # record `x` to store the population. Needed for constructing OptimizationState.
    dt["x"] = population
    
    # set additional trace value
    Evolutionary.trace!(dt, objfun, state, population, method, options)
    Evolutionary.update!(tr,
            state,
            iteration,
            Evolutionary.value(state),
            dt,
            options.store_trace,
            options.show_trace,
            options.show_every,
            options.callback)
    end
  4. The added test tests that "x" as well as example user-defined trace record key are present in the output, which passed.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (fedb001) 8.69% compared to head (cf6caa2) 8.68%.

Files Patch % Lines
...zationEvolutionary/src/OptimizationEvolutionary.jl 0.00% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #702 +/- ## ========================================= - Coverage 8.69% 8.68% -0.02% ========================================= Files 31 31 Lines 2496 2500 +4 ========================================= Hits 217 217 - Misses 2279 2283 +4 ```

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

jonathanfischer97 commented 4 months ago

@Vaibhavdixit02 Agreed, just wanted to keep the changes limited for this PR. No problem!

Vaibhavdixit02 commented 4 months ago

Why did you close this @jonathanfischer97?

jonathanfischer97 commented 4 months ago

@Vaibhavdixit02 Oops, my bad, thought branch was merged. Still getting the hang of contributing to public repos!

Vaibhavdixit02 commented 4 months ago

OptimizationEvolutionary test failure looks real, can you take a look at it?

jonathanfischer97 commented 4 months ago

@Vaibhavdixit02 Will do!

jonathanfischer97 commented 4 months ago

@Vaibhavdixit02 Fixed the trace test, previous test was querying the trace of sol which wasn't storing trace higher up in the testset. So now sol is the solution from the solve testing the callback and trace functionality. Should pass now!

Vaibhavdixit02 commented 4 months ago

Thanks, this is great!