-
Notifications
You must be signed in to change notification settings - Fork 35
Add an API for n-th order tensor products #486
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
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.
Thanks guys for starting this! This is a first batch of comments.
src/nlp/api.jl
Outdated
""" | ||
P = tensor_projection(nlp, n, x, directions, args...) | ||
|
||
Returns the projection of the n-th derivative of the objective of `nlp` at `x` along the specified directions. |
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.
What about tensor_projection
?
You should also have
P = tensor_projection(nlp, n, x, y, directions, args...)
as the default, where by default
P = tensor_projection(nlp, n, x, directions, args...)
call the first one with zeros(n).
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.
I agree that it will be great to have tensor products with both the objective and the Lagrangian.
Should we also add an API for just the constraints, or include a scaling factor \alpha
that can go in front of the objective?
\alpha
can be zero.
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.
I think it makes sense to be coherent with the other functions like hprod
.
src/nlp/api.jl
Outdated
) where {T, S} | ||
@lencheck nlp.meta.nvar x | ||
m = n - length(directions) | ||
@assert m ≥ 1 |
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.
Can we use NLPModels.@rangecheck instead of @Assert ?
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.
Can we use rangecheck
to ensure inequalities?
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.
Not tested, but I would expect @rangecheck 1 typemax(Int) m
to work
cc @michel2323