tBuLi
commented
6 years ago So far, I really like what you've done with this one! Looking forward to docs and tests ;).
Just one critical note, I'm not sure I like the factory function for ChainedMinimizer, wouldn't it make more sense to just have __init__ accept a minimizers (keyword) argument? e.g.
def f(x, a, b):
return a * x + b
def chi_squared(a, b):
return np.sum((ydata - f(xdata, a, b))**2)
custom_minimize = ChainedMinimizer(chi_squared, [a, b], minimizers=[DifferentialEvolution, SLSQP])
fit_results = custom_minimize.execute()I think this looks cleaner than
CustomMinimizer = ChainedMinimizer([DifferentialEvolution, SLSQP])
custom_minimize = CustomMinimizer(chi_squared, [a, b])And I wrote fit_results in plural because I think it would help users for debugging purposes if this method returned all the FitResults, as a list.
As some final icing on the cake, we can have Fit accept a list for the minimizer argument, which we will then simply wrap internally. This would then for most use cases reduce the above code to
x, y = variables('x, y')
a, b = parameter('a, b')
model = {y: a * x + b}
fit = Fit(model, x=xdata, y=ydata, minimizer=[DifferentialEvolution, SLSQP])
fit_results = fit.execute()minimizer_kwargs could then be passed as
fit_results = fit.execute(DifferentialEvolution={'popsize': 100}, SLSQP={'tol': 1e-9})(this last part about Fit should perhaps be done as a seperate PR)
pckroon
Jhsmit
So this adds a Differential Evolution minimizer, which is awesome. I still need to do docstrings, tests and possibly documentation.
You can already look at the code though.