-
Notifications
You must be signed in to change notification settings - Fork 16
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
[documentation] Update the performance tips #328
Conversation
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.
@amontoison nice. regarding the syntax, it's getting a bit heavy. though it does not seem to be standard in Julia yet, I'd advocate sth like what used to be done, e.g., in matlab: build iteratively the options using an ad hoc constructor, then, pass it to the code. or having a dedicated keyword for such "empty" backends...
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.
@tmigot Any idea of how we can design that with the current API?
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.
@jbcaillau do you mean having something like:
ADNLPModel(f, x0, c, lcon, ucon, gradient_backend)
ADNLPModel(f, x0, c, lcon, ucon, gradient_backend, jacobian_backend)
ADNLPModel(f, x0, c, lcon, ucon, gradient_backend, jacobian_backend, jprod_backend, jtprod_backend)
ADNLPModel(f, x0, c, lcon, ucon, gradient_backend, jacobian_backend, jprod_backend, jtprod_backend, hessian_backend)
...
I find it kind of heavy and it will not be 100% clear or do you mean something different ?
Note that in the code right now, all these backends objects all are subtypes of ADBackend, so we don't have ways to distinguish between different types of backends without keywords.
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.
it s ok like that
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.
Thanks @amontoison , LGTM
@jbcaillau