Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow curve_fit() to take a data argument to be passed to model function #14

Closed
wants to merge 2 commits into from

Conversation

olehmer
Copy link

@olehmer olehmer commented Apr 18, 2016

When using the curve fit function I found the need to pass constant arguments to the model function. Using global variables was doable but unfortunate. The margs=[] argument will work with all legacy code using the curve_fit() function but allow future calls to handle a wider range of use cases.

@mlubin
Copy link
Contributor

mlubin commented Apr 18, 2016

Thanks for taking the time to put together this contribution, but this issue has been discussed at length:
JuliaNLSolvers/Optim.jl#102
JuliaNLSolvers/Optim.jl#53
JuliaNLSolvers/Optim.jl#94
JuliaNLSolvers/Optim.jl#85
and the conclusion is to use Julia's closure functionality, as described at:
https://github.com/JuliaOpt/Optim.jl#optimizing-functions-that-depend-on-constant-parameters
Closures have no performance penalty in Julia 0.5 and later.

@ChrisRackauckas
Copy link
Contributor

You could also use call overloading and have model be a type with a field for the data. That would allow you have the data "explicit" and accessible (unlike a closure), but also requires no API change. It's how parameters for differential equations are handled in JuliaDiffEq (ParameterizedFunctions.jl)

@blakejohnson
Copy link
Contributor

I agree with @mlubin, I think closures are a much better way to handle this scenario, especially now that closures have no overhead in julia 0.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants