-
Notifications
You must be signed in to change notification settings - Fork 37
Update jac_lin_coord
methods
#501
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
Conversation
@arnavk23 It is a breaking change if we drop the signature with |
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 @arnavk23 for the PR.
Can you also adapt jac_lin_op
and jac_lin_op!
https://github.com/arnavk23/NLPModels.jl/blob/504a3c29574998b892d7f598283db466a86e391e/src/nlp/api.jl#L737
@tmigot All test have passed except the breakage/upload one. |
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 @arnavk23 for these changes. I gave some thoughts about @amontoison 's comment, and it's a good idea to deprecate these functions instead of removing them directly.
Using the macro @deprecate when somebody uses the "old" variant it should receive a warning saying that the function is deprecated and point to the variant.
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 @arnavk23 for the changes. I made some comments.
I think we also miss the functions jprod
and jtprod
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
We also need to remove the |
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
Co-authored-by: Tangi Migot <[email protected]>
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 @arnavk23 !
The linear Jacobian part is independent of
x
, so passing it became unnecessary.jac_lin_coord(nlp, x)
→jac_lin_coord(nlp)
jac_lin_coord!(nlp, x, vals)
→jac_lin_coord!(nlp, vals)
Closes #404