Support optimization backends via package extensions #228
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #228 +/- ##
==========================================
- Coverage 74.14% 73.28% -0.87%
==========================================
Files 52 49 -3
Lines 2197 2231 +34
==========================================
+ Hits 1629 1635 +6
- Misses 568 596 +28 ☔ View full report in Codecov by Sentry. |
|
@Maximilian-Stefan-Ernst Also, I haven't touched the sources layout, but you may consider merging the optimizer parts in |
|
I think that's definitely a good idea, the "diff"-folder is a misnomer anyways^^ |
|
@Maximilian-Stefan-Ernst I've moved the code from the |
|
I finally got to the review, thanks for the great changes (again)! I've made some minor comments, I think the only thing that would be left before merging would be to move the tests from the ProximalSEM.jl package to the tests of this package. |
|
I moved the tests from ProximalSEM. |
|
@Maximilian-Stefan-Ernst Thank you for a thorough review and the fixes! I will address the remaining ones in 1-2 days. BTW, have you tried/considered using Manopt.jl for SEM optimization? |
|
@Maximilian-Stefan-Ernst I think I have addressed all of your comments, thanks again for the review! |
to consolidate ENV variable check
* convert to argument type-dependent dispatch * replace start_val() function with prepare_start_params() * refactor start_parameter_table() into prepare_start_params(start_val::ParameterTable, ...) and use the SEM model param indices * unify processing of starting values by all optimizers * support dictionaries of values
Co-authored-by: Maximilian Ernst <[email protected]>
NLopt minimum was 18.11, below what the test expected
alyst
force-pushed
the
optim_extensions
branch
from
e8842a0 to
c5b48c7
Compare
|
I've cleaned up the commit history, so it should be ok to merge if no other changes are required. |
Maximilian-Stefan-Ernst
merged commit b839635
into
StructuralEquationModels:devel
|
@alyst: W.r.t. Manopt.jl: The problem why I did not manage to implement the SPD constraint with ProximalAlgorithms.jl is because we have this situation where the model-implied covariance matrix is a function of the parameters, so we have parameters -> cov matrix -> loss, and we have to constrain in the middle instead of the parameter space. I am not sure how we could alleviate this with Manopt - I believe we would have to figure out the manifold of the parameter space? |
This is another set of changes from #193 and my staging branch:
optimizeris removed from theSemobject.Semdefines the SEM model, which is independent from how the parameters of this model are fit.Now the optimizer has to be specified for
sem_fit()call.(I guess the optimizer was a part of
Sem, because some optimizers allow specifying parameter constraints, which is the part of the model.But that constraints specification was optimizer-dependent. In future, it should be possible to implement optimizer-agnostic constraints specification.)
That change simplifies the code a bit, since
Sem.optimizerwas never really used except to callsem_fit().Plus, it allows fitting the same model with different optimizer backends without recreating the
Semobject.Implementation details:
SemOptimizer{E}: engine type parameterEwas added to the abstractSemOptimizerclass,i.e.
SemOptimizer{E}(args...; kwargs...)constructs the optimizer object for theEbackend (e.g.E = :Optimfor Optim.jl).SemOptimizer{E}(...)constructor, e.g.SemOptimizer{:Optim}(...)createsSemOptimizerOptim <: SemOptimizer{:Optim}object;sem_fit(optim::O, model, ...) where O <: SemOptimizer{E}callsem_fit(model; engine, kwargs...), which first createsSemOptimizer{engine}(;kwargs...)optimizer and then calls itssem_fit()E = :Optim), so it remains a hard dependency of SEM.jlE = :NLopt) is moved into theSEMNLOptExtextension (it gets automatically activated once the NLopt package is loaded into an activate session)SEMProximalOptExtextension (E = :Proximal) to enable the ProximalAlgorithms.jl backend support.sem_fit()for the Optim.jl backend.