Change controller handling with the adaptive option

[Theozeud]

Checklist

• Appropriate tests were added
• Any code changes were done in a way that does not break public API
• All documentation related to code changes were updated
• The new code follows the
  contributor guidelines, in particular the SciML Style Guide and
  COLPRAC.
• Any new documentation only uses public API

Additional context

One can now use a controller with the option adaptive set to false.

[ranocha]

Why does this file exist at all?

[ChrisRackauckas]

It looks like this was a mistake in the big refactors, we should delete it.

[ranocha]

This will introduce an additional type instability since the type of controller is determined by the value of adaptive. I cannot really judge how severe this issue is right now. Others probably have more to say about this.

[Theozeud]

Why is there a type instability ?

[ranocha]

julia> function foo(; adaptive = true)
           if adaptive
               return 1.0
           else
               return 1
           end
       end
foo (generic function with 1 method)

julia> @code_warntype foo(adaptive = true)
MethodInstance for Core.kwcall(::@NamedTuple{adaptive::Bool}, ::typeof(foo))
  from kwcall(::NamedTuple, ::typeof(foo)) @ Main REPL[1]:1
Arguments
  _::Core.Const(Core.kwcall)
  @_2::@NamedTuple{adaptive::Bool}
  @_3::Core.Const(foo)
Locals
  adaptive::Union{}
  @_5::Bool
Body::Union{Float64, Int64}
1 ─       Core.NewvarNode(:(adaptive))
│         Core.NewvarNode(:(@_5))
│   %3  = Core.isdefined(@_2, :adaptive)::Core.Const(true)
└──       goto #3 if not %3
2 ─       (@_5 = Core.getfield(@_2, :adaptive))
└──       goto #4
3 ─       Core.Const(:(@_5 = true))
4 ┄ %8  = @_5::Bool
│   %9  = (:adaptive,)::Core.Const((:adaptive,))
│   %10 = Core.apply_type(Core.NamedTuple, %9)::Core.Const(NamedTuple{(:adaptive,)})
│   %11 = Base.structdiff(@_2, %10)::Core.Const(NamedTuple())
│   %12 = Base.pairs(%11)::Core.Const(Base.Pairs{Symbol, Union{}, Tuple{}, @NamedTuple{}}())
│   %13 = Base.isempty(%12)::Core.Const(true)
└──       goto #6 if not %13
5 ─       goto #7
6 ─       Core.Const(:(Base.kwerr(@_2, @_3)))
7 ┄ %17 = Main.:(var"#foo#1")(%8, @_3)::Union{Float64, Int64}
└──       return %17

[ranocha]

Note the Union{Float64, Int64}

[Theozeud]

Ok, but is there no type instability due to the fact that the type of the controller depends also on the algorithm is adaptive is true ?

[Theozeud]

@ChrisRackauckas @oscardssmith What do you think about this ?

[oscardssmith]

it is a minor problem. Are there settings of the normal PI controllers that will make them not adapt ever? It feels like there should be some combination of 0 and Inf that make it never adapt.

[Theozeud]

I do not know, is it related to my changes ?

[ChrisRackauckas]

A better strategy would be to add traits to controllers for overrides_adaptive(controller) or something, which would allow the default controller to stay the same (making it better for compilation) while allowing the alternative controllers to bypass the if adaptive checks by making them if adaptive || overrides_adaptive(controller).

[Theozeud]

@ChrisRackauckas I thought over this but for each things I tried, I do not succeed to prevent from a type instability. Can you explain a little more ? I think I have missunderstood something but I do not know what.