-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
re-establish build_lsoptim_objective() #221
base: master
Are you sure you want to change the base?
Conversation
@@ -31,6 +31,7 @@ julia = "1.6" | |||
BlackBoxOptim = "a134a8b2-14d6-55f6-9291-3336d3ab0209" | |||
DelayDiffEq = "bcd4f6db-9728-5f36-b5f7-82caef46ccdb" | |||
ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210" | |||
LeastSquaresOptim = "0fc2ff8b-aaa3-5acd-a817-1944a5e08891" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it an extension package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even this is not needed - LeastSquaresOptim is only in the weakdeps for testing.
Since the change was made in a major release it shouldn't be breaking code, unless you updated the package purposefully of course. In which case if you need the functionality it should be fine to use the older version of the package? |
Sure. But the argument is - why remove this possibility when it just is implemented with one short function and does not even create a dependency or need an extension ? IMHO this just decreases composability with no gains or otherwise pins packages (and teaching examples etc) to old versions. |
No gains? There's some very clear gains in terms of maintainability, composability, clarity, and documentation in that update. DiffEqParamEstim updated to re-target towards the Optimization.jl interface rather than having its own weird hybrid of AD support and abstract optimization library targeting. By doing so, it cut out a whole lot of code and centralized around what is now a well-documented optimizer interface. Thus all of the small objective functions were kicked out. Why should a parameter estimation package have to bend to the API of every optimization library, especially when there is an optimization library that covers all of these interfaces? Instead of having one hacky workaround here that makes this work but for example doesn't make DiffEqFlux compatible with it, LeastSquaresOptim should be added to Optimization.jl's interface. That would:
That's a pretty major improvement and I don't think we want to compromise with a hacky single function that goes around the centralized APIs. As things evolve things have become more structured and DiffEqParamEstim was just the outlier that was behind in adopting the more general structures.
That's fine, manifests still work. But for everyone else, we very much hope they are teaching by using a single documented Optimization.jl package rather than an undocumented collogue of optimization packages with subtly different interfaces. I think clear documentation would serve students best. |
Hi,
IMHO removing
build_lsoptim_objective()
breaks existing code unnecessarily. Moreover, this used to be in usage examples, and I did not find a clear upgrade recipe to the current version.