fix argument order to cons_vjp
#98
Conversation
Bumps [crate-ci/typos](https://github.com/crate-ci/typos) from 1.23.6 to 1.24.1. - [Release notes](https://github.com/crate-ci/typos/releases) - [Changelog](https://github.com/crate-ci/typos/blob/master/CHANGELOG.md) - [Commits](crate-ci/typos@v1.23.6...v1.24.1) --- updated-dependencies: - dependency-name: crate-ci/typos dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
…e-ci/typos-1.24.1 Bump crate-ci/typos from 1.23.6 to 1.24.1
|
Can you change the target branch to v2 #96 |
| end | ||
| elseif cons_vjp == true && cons !== nothing | ||
| cons_vjp! = (J, θ, v) -> f.cons_vjp(J, θ, v, p) | ||
| cons_vjp! = (J, v, θ) -> f.cons_vjp(J, v, θ, p) |
There was a problem hiding this comment.
Actually, I don't think this is correct. We don't want to have this order as API too since it'll be inconsistent with jvp and be higher mental load.
What makes you think the current implementation is incorrect? https://gdalle.github.io/DifferentiationInterface.jl/DifferentiationInterface/dev/api/#DifferentiationInterface.pullback and https://github.com/jump-dev/MathOptInterface.jl/blob/cb1ad41c3a5ce1d12e83f2be60f10b191b0e22c5/src/nlp.jl#L762-L809 both match my understanding as well
I did spend a bit of time making sure the dimensions here were correct
There was a problem hiding this comment.
When you call DI, the argument order is pullback!(cons_oop, J, adtype, θ, v, extras_pullback), but in SciML, the argument order is
- `cons_jvp(Jv,v,x,p)` or `Jv=cons_jvp(v,x,p)`: the Jacobian-vector product of the constraints.
- `cons_vjp(Jv,v,x,p)` or `Jv=cons_vjp(v,x,p)`: the Jacobian-vector product of the constraints.https://github.com/SciML/SciMLBase.jl/blob/master/src/scimlfunctions.jl#L1833C1-L1834C96
There was a problem hiding this comment.
Ah lol so I missed the reference closest to home, I'll change that, this order makes less sense to me than the one used here. What do you think?
There was a problem hiding this comment.
I don't have a strong opinion here. I'd almost expect v'J(x) to take args v, x and vice versa for J(x)*v, but having them be different might also be confusing. As long as SciML is internally consistent everywhere it can be whatever you prefer.
hv(Hv,u,v,p) or Hv=hv(u,v,p): the Hessian-vector product (d^2 f / du^2) v.takesvafteru, maybe change utox` to be consistent as well?
There was a problem hiding this comment.
makes sense, thanks for pointing these out 🙌
|
The order of the argument in SciMLBase has been fixed so this isn't needed anymore, thanks for catching this @baggepinnen |
3 participants
The SciML function and the corresponding DifferentiationInterface function have different argument order