Use _SubexpressionStorage inside _FunctionStorage

[blegat]

No description provided.

[odow]

In addition to getting these tests passing, we also need to run solver-tests.yml.

[odow]

This is only marginally a net win for lines of code. I'd err towards not merging this as-is. I'm all for cleaning up the ReverseAD implementation, but I'm also quite wary of making changes. The last release showed how easy it is to make a tiny mistake that isn't caught by the current tests.

[blegat]

The fact that we get rid of these warnings is a good sign

[blegat]

I'm not planning much deeper refactoring but I think this one helps a lot in readability and simplifies the logic.
It's not really about the number of lines, it's more about the overall readability.

[odow]

https://github.com/jump-dev/MathOptInterface.jl/actions/runs/14919497155

[blegat]

This broke EAGO because it is accessing the fields of _FunctionStorage here:
https://github.com/PSORLab/EAGO.jl/blob/238966d863153d47b6fc02a0422666760f1d6e7f/src/eago_optimizer/functions/nonlinear/graph/graphs/directed_tree.jl#L232-L234

[odow]

Breaking a solver, even if it was using private API, is a reason to close this PR. I really don't think we need to make changes to ReverseAD unless necessary.

[blegat]

We can overload Base.getproperty to make it non-breaking. Once we have #2750, we can add in the comment of getproperty that it's just for backward compat and then in a later release we can warn and then in a later release we can delete.
But I don't think it's the first time we changed something internally and broke a package. If a package is using MOI's internal, it should pin MOI's version.
I still feel that we need this. I know refactoring something that works is adding risks so it should better be worth it but I think the benefits outweighs the risk here. Adding a little bit of encapsulation will simplify the source code.
It's also adding coverages to _SubexpressionStorage. At the moment, since JuMP does not use _SubexpressionStorage, it is not covered so much. At least with this PR, both the expressions of constraint, the objective and subexpressions end-up in the same code-path down the line which is good.
The only difference between subexpressions and objective/constraints was that subexpression cannot share the partials_storage_ϵ. I don't think that it is enough to justify duplicating all the fields in the two structs. We can just create a shared vector in MOI.initialize.

[odow]

I don't want to complicate things by overloading getproperty. And it still means that we'd break EAGO for some versions of of MOI and EAGO.

Perhaps we need to talk in person to understand the motivation for all this work on ReverseAD.

I'd strongly prefer that we just left it alone.

If you're interested in writing a new AD engine, prototype it in a separate package. We can swap AD backends now, so we don't need to make changes to ReverseAD.

[blegat]

The motivation is that I'd like to add support for vector AD in ReverseAD. I actually think that most of the design of ReverseAD will fit quite well for the vector version as well so I wanted to see if it could just be added on top. As it will need to slightly complicate it, I wanted to do small refactoring to simplify it before, and that was also helped me understand it better. But you're right, I'll start it as a separate package.

[blegat]

We can close as it was moved to blegat/ArrayAD.jl#3