Sync API is cumbersome (in my opinion)

[kylebeggs]

If my understanding is correct, to add a sync function to a GenericSplitFunction, you need to overload synchronize_solution_with_parameters!(integrator, params, sync::MySync) where MySync is some type the user must define. To be concrete, at minimum I need to define all the below:

struct MySync end
sync = MySync()

function OrdinaryDiffEqOperatorSplitting.synchronize_solution_with_parameters!(
    outer_integrator::OrdinaryDiffEqOperatorSplitting.OperatorSplittingIntegrator,
    ::SciMLBase.NullParameters,
    ::MySync
)
    @info "Sync called at time $(outer_integrator.t)"
end

I feel this is a bit cumbersome and would like to suggest just a function-based approach where the user just needs to supply a function directly such as

my_sync(outer_integrator, params) = ...

We would just need to add

function synchronize_solution_with_parameters!(outer_integrator::OperatorSplittingIntegrator, p, sync::F) where {F<:Function}
    sync(outer_integrator, p)
end

and we could still keep the other way as well. This should be OK since the functions should be known at compile time.

Am I missing something why this can't work? Or will hamper other wanted behavior?

[termi-official]

I am not sure I am understanding the problem here. If you have no parameters, i.e. SciMLBase.NullParameters, then you should not need to dispatch anything due to the following dispatch

https://github.com/SciML/OrdinaryDiffEqOperatorSplitting.jl/blob/650ca71e9ab04ca3b3b760787aaa5311e991b6da/src/utils.jl#L101C1-L101C131

Can you append the error message you get?

Regarding the design, the parameter object might be either not sufficient to control the dispatch logic as you would usually do in operator splitting. Yes, you can potentially define your parameters to be the sync object, but this complicates things downstream when interfacing with SciML, because you need to unpack the sync object into the "original parameters" at some places. The reason why I decided to use a struct+dispatch instead of a callable struct is personal preference for now. Maybe you can help me here with the design, as I am really not understanding what causes trouble here yet. Can you elaborate a bit on waht exactly the advantage on doing

struct MySyncFunction
    ...
end
(p::MySyncFunction)(outer_integrator, params) = ...

over

struct MySyncStruct
    ...
end
synchronize_solution_with_parameters!(outer_integrator, params, sync::MySyncStruct) = ...

is?

[kylebeggs]

I am not sure I am understanding the problem here. If you have no parameters, i.e. SciMLBase.NullParameters, then you should not need to dispatch anything due to the following dispatch

Indeed, this is fine.

The reason why I decided to use a struct+dispatch instead of a callable struct is personal preference for now. Maybe you can help me here with the design, as I am really not understanding what causes trouble here yet. Can you elaborate a bit on waht exactly the advantage on doing

This issue is not a bug report - its just a style/API gripe. The difference between the two approaches is the user needs to figure out they need to overload synchronize_solution_with_parameters! to get this to work. Sure, we can just add this to the docs and make it more visible for them, but it would be even better if you allow them to define their own functions as well. I think there is less friction for the user that way. We could probably support both APIs actually.