Issue
- The
Algorithm structure currently holds a State object. This design is counterintuitive as Algorithm should ideally represent static parameters, options, and optimization criteria, while State should hold mutable information updated during the algorithm's execution.
- This encapsulation of
State within Algorithm can lead to confusion about the responsibilities of these two structures and unintended side effects.
Unnecessary encapsulation
In the main optimization loop, most operations directly interface with State, suggesting that the current encapsulation in Algorithm isn't fully exploited or even necessary.
For instance, the update_state! function is passed all the fields of Algorithm separately, which seems to defeat the purpose of having State encapsulated within Algorithm.
Here's an example from during.jl:
function _during_optimization!(status, parameters, problem, information, options, convergence, logger, termination)
...
update_state!(status, parameters, problem, information, options)
...
endThis is an indication that State is already being treated as an independent object in the source code, and doesn't need any relation to Algorithm. As such, a refactor wouldn't require extensive changes from what I've seen.
User Impact
This issue became apparent when using the package in a typical way:
# Define method
de = DE(;N = 10000, F_min = 0.5, F_max =1.0, strategy = :best1, options = options)
# Run optimization and return final state
de_optimization = optimize(objective_parallel, bounds, de, logger=logger)
-
In this example, de is assumed to hold static optimization parameters that can be reused. However, it also holds State in its status field, which is not obvious unless one examines the source code.
- As a side note, it's also not obvious that the
DE constructor returns an Algorithm rather than the type DE <: AbstractDifferentialEvolution that it shares a name with. I can understand why this construction was done, but I think this is atypical and another potential source of confusion.
-
Because de is mutated during the optimize call, if the optimization is run again after a previous run without reinitializing de, the optimization starts the search from the previous best solution, rather than starting from a clean slate.
-
This can lead to unintended side effects for users trying to run benchmarks or comparisons between different optimization options, such as I was.
Proposed Solution
Consider refactoring the code to separate these two concepts more clearly.
This could involve removing the State object from Algorithm and instead passing it as a separate argument to the functions that need to mutate it. Specifically within src/optimize:
-
The user only passes the Algorithm method, without the status field, to optimize.
-
optimize calls _before_optimization!, which creates a new State object, rather than mutating with initialize!(method.status, ...) as it does currently.
-
_during_optimization! and _after_optimization! are passed status separately from the other Algorithm properties, which they already do, so no changes needed after initialization!
I'd be interested to hear your thoughts on this. If you agree with this assessment, I'd be happy to help with the refactoring process, as like I said, there don't seem to be many dependencies on Algorithm anyways.
Issue
Algorithmstructure currently holds aStateobject. This design is counterintuitive asAlgorithmshould ideally represent static parameters, options, and optimization criteria, whileStateshould hold mutable information updated during the algorithm's execution.StatewithinAlgorithmcan lead to confusion about the responsibilities of these two structures and unintended side effects.Unnecessary encapsulation
In the main optimization loop, most operations directly interface with
State, suggesting that the current encapsulation inAlgorithmisn't fully exploited or even necessary.For instance, the
update_state!function is passed all the fields ofAlgorithmseparately, which seems to defeat the purpose of havingStateencapsulated withinAlgorithm.Here's an example from
during.jl:This is an indication that
Stateis already being treated as an independent object in the source code, and doesn't need any relation toAlgorithm. As such, a refactor wouldn't require extensive changes from what I've seen.User Impact
This issue became apparent when using the package in a typical way:
In this example,
deis assumed to hold static optimization parameters that can be reused. However, it also holdsStatein its status field, which is not obvious unless one examines the source code.DEconstructor returns anAlgorithmrather than the typeDE <: AbstractDifferentialEvolutionthat it shares a name with. I can understand why this construction was done, but I think this is atypical and another potential source of confusion.Because
deis mutated during theoptimizecall, if the optimization is run again after a previous run without reinitializingde, the optimization starts the search from the previous best solution, rather than starting from a clean slate.This can lead to unintended side effects for users trying to run benchmarks or comparisons between different optimization options, such as I was.
Proposed Solution
Consider refactoring the code to separate these two concepts more clearly.
This could involve removing the
Stateobject fromAlgorithmand instead passing it as a separate argument to the functions that need to mutate it. Specifically withinsrc/optimize:The user only passes the
Algorithmmethod, without thestatusfield, tooptimize.optimizecalls_before_optimization!, which creates a newStateobject, rather than mutating withinitialize!(method.status, ...)as it does currently._during_optimization!and_after_optimization!are passedstatusseparately from the otherAlgorithmproperties, which they already do, so no changes needed after initialization!I'd be interested to hear your thoughts on this. If you agree with this assessment, I'd be happy to help with the refactoring process, as like I said, there don't seem to be many dependencies on
Algorithmanyways.