Skip to content

Conversation

SebastianM-C
Copy link
Contributor

@SebastianM-C SebastianM-C commented Oct 20, 2025

I'm splitting up the changes to OptimizationBase from #1067 here

I think we can also do the supports_opt_cache_interface cleanup in this PR too.

After SciML/SciMLBase.jl#1157 merges we can replace supports_opt_cache_interface with the new API.

@SebastianM-C SebastianM-C marked this pull request as ready for review October 22, 2025 13:08
@SebastianM-C
Copy link
Contributor Author

@ChrisRackauckas is this one okay?

@SebastianM-C
Copy link
Contributor Author

Should I change the CI setup so that Core uses the PR version of OptimizationBase instead of the release, now that most things are in OptimizationBase?
Currently the CI doesn't really test OptimizationBase changes at all 😅

@ChrisRackauckas
Copy link
Member

It's supposed to via sources, but yeah 😅 changing the CI could help

@SebastianM-C
Copy link
Contributor Author

We can't really test the Core group without the updates to OptimizationOptimJL and the other solvers used, should I add the solver updates here or do we merge this and then continue in #1067 ?

Btw, it looks like we were not running the OptimizationBase tests at all 😅

@ChrisRackauckas
Copy link
Member

That's fine, because OptimizationBase.jl cannot really be used without a solver. So the real thing is the bump of the solver compats for this.

@SebastianM-C
Copy link
Contributor Author

SebastianM-C commented Oct 22, 2025

well, since this is a breaking change, I think we're fine to merge if there isn't anything else to add here.
With how the CI is setup we will need to merge an release v4 and then do #1067

@ChrisRackauckas
Copy link
Member

I'd just make sure we have an easy way to also dispatch on in placeness of f while we're at it, and good to go.

@SebastianM-C
Copy link
Contributor Author

I'm not sure I fully understand what should I add 😅

For OptimizationBase.instantiate_function we have parametric dispatch on the iip type parameter for OptimizationFunction for

  • OptimizationEnzymeExt.jl
  • OptimizationDIExt.jl
  • OptimizationDISparseExt.jl

and for

  • OptimizationZygoteExt.jl
  • OptimizationMTKExt.jl

we don't have the out of place variant. Should I (ask claude to) add those?

As far as I can tell all optimizers use the in-place version, so the out of place stuff is never used. I would assume that it could be interesting for applications that don't support mutation, but in that case we would need support from the optimizers (do we need a trait for that?). Otherwise if we implement the closure passed to the optimizer as in-place based on an out of place Optimization function, we would have to more or less duplicate all implementations.

There's also the MultiObjective stuff, but I would assume that it would need #1017 merged first, we have there the option of having an in-place objective function, and that would also need to pass the appropriate iip/oop function to the underlying optimizer.

@ChrisRackauckas
Copy link
Member

Static and GPU kernel optimizers will need it. So OptimizationFunction has an IIP, and we should make it easy to dispatch the cache on that.

@SebastianM-C
Copy link
Contributor Author

so something like

isinplace(::OptimizationBase.OptimizationCache{O, OptimizationFunction{iip}}) where {O, iip} = iip

?

@ChrisRackauckas
Copy link
Member

but lift that type parameter to the OptimizationCache as the second type parameter

@SebastianM-C
Copy link
Contributor Author

done & rebased on master

@ChrisRackauckas ChrisRackauckas merged commit dbeed2d into SciML:master Oct 23, 2025
4 of 81 checks passed
@SebastianM-C SebastianM-C deleted the optbase_cleanup branch October 23, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants