Missing tests

[baggepinnen]

I noticed that there are still some tests missing since the recent refactor

• The tests for the previous Saturation, now Limiter are missing.
• The tests for SlewRateLimiter does not test that the slew rate of a signal is limited.
• The tests for LimPID are deactivated. I tried to update this one but could no longer easily follow the signal in the PID controller. Explanation below.
• In general, several tests that previously tested that the result of simulating blocks were correct are missing and replaced by trivial tests that test a single value in the solution.

The previous implementation of the PID controller

    if isequal(Ti, false)
        @named I = Gain(false)
    else
        @named I = Integrator(k = 1/Ti)
    end
    @named sat = Saturation(; y_min, y_max)
    derivative_action = Td > 0
    eqs = [
        e ~ u_r - u_y # Control error
        ep ~ wp*u_r - u_y  # Control error for proportional part with setpoint weight
        ea ~ sat.y - sat.u # Actuator error due to saturation
        I.u ~ e + 1/(k*Ni)*ea  # Connect integrator block. The integrator integrates the control error and the anti-wind up tracking. Note the apparent tracking time constant 1/(k*Ni), since k appears after the integration and 1/Ti appears in the integrator block, the final tracking gain will be 1/(Ti*Ni) 
        sat.u ~ derivative_action ? k*(ep + I.y + D.y) : k*(ep + I.y) # unsaturated output = P + I + D
        y ~ sat.y
    ]
    systems = [I, sat]
    if derivative_action
        push!(eqs, ed ~ wd*u_r - u_y)
        push!(eqs, D.u ~ ed) # Connect derivative block
        push!(systems, D)
    end

has been replaced by

if with_I
        @named addI = Add3(k1=1, k2=-1, k3=1)
        @named int = Integrator(k=1/Ti, x_start=xi_start)
        @named limiter = Limiter(y_max=u_max, y_min=u_min)
        @named addSat = Add(k1=1, k2=-1)
        @named gainTrack = Gain(1/(k * Ni))
    else
        @named Izero = Constant(k=0)
    end
    if with_D
        @named der = Derivative(k=1/Td, T=1/Nd, x_start=xd_start)
        @named addD = Add(k1=wd, k2=-1)
    else
        @named Dzero = Constant(k=0)
    end

    sys = [reference, measurement, ctr_output, addP, gainPID, addPID]
    if with_I
        push!(sys, [addI, int, limiter, addSat, gainTrack]...)
    else
        push!(sys, Izero)
    end
    if with_D
        push!(sys, [addD, der]...)
    else
        push!(sys, Dzero)
    end

    eqs = [
        connect(reference, addP.input1),
        connect(measurement, addP.input2),
        connect(addP.output, addPID.input1),
        connect(addPID.output, gainPID.input),
    ]
    if with_I
        push!(eqs, connect(reference, addI.input1))
        push!(eqs, connect(measurement, addI.input2))
        push!(eqs, connect(gainPID.output, limiter.input))
        push!(eqs, connect(limiter.output, ctr_output))
        push!(eqs, connect(limiter.input, addSat.input2))
        push!(eqs, connect(limiter.output, addSat.input1))
        push!(eqs, connect(addSat.output, gainTrack.input))
        push!(eqs, connect(gainTrack.output, addI.input3))
        push!(eqs, connect(addI.output, int.input))
        push!(eqs, connect(int.output, addPID.input3))
    else
        push!(eqs, connect(Izero.output, addPID.input3))
    end
    if with_D
        push!(eqs, connect(reference, addD.input1))
        push!(eqs, connect(measurement, addD.input2))
        push!(eqs, connect(addD.output, der.input))
        push!(eqs, connect(der.output, addPID.input2))
    else
        push!(eqs, connect(Dzero.output, addPID.input2))
    end

which in my opinion totally obfuscates what is going on. The trivial blocks likely exists in modelica stdlib and simulink because they may be useful when building models graphically, they may not be the best choice to implement systems in code. The new implementation requires a block diagram to follow and I would suggest we opt for an implementation style that is slightly easier to maintain.

[ValentinKaisermayer]

Thanks for the issue!

• There are tests for the Limiter:
  

  ModelingToolkitStandardLibrary.jl/test/Blocks/nonlinear.jl

  Line 7 in 4b081e6

  @testset "Limiter" begin

  
  What additional test would you think is needed?

• The SlewRateLimiter needs indeed a test for the output slew rate.

• There is a basic test for the LimPID :
  

  ModelingToolkitStandardLibrary.jl/test/Blocks/continuous.jl

  Line 289 in 4b081e6

  @testset "LimPID" begin

The LimPID implementation is from Modelica:

But you are right all the trivial Add and Gain blocks may be more confusing than helping. This is a design decision I think.

[baggepinnen]

There are tests for the Limiter:

My bad, I missed it. I was comparing with the tests I had locally, that tested the entire trajectory:

    y = clamp.(sin.(sol.t), y_min, y_max)
    @test sol[sat.y] ≈ y rtol = 1e-6

so this is another case where a thorough test has been replaced by a test that could pass with arbitrarily poor result of the simulation.

The current test of the PID controller is simply

@test sol[ref.output.u - plant.output.u][end] ≈ 0 atol=1e-3 # zero control error after 100s

which could easily pass if the entire solution was just zeros. Compare with the previous tests

# linearity in u_r
    controller, sys, sol1 = solve_with_input(u_r=sin(t), u_y=0)
    controller, sys, sol2 = solve_with_input(u_r=2sin(t), u_y=0)
    @test sum(abs, sol1[controller.ea]) < eps() # This is the acutator model error due to saturation
    @test 2sol1[controller.y] ≈ sol2[controller.y] rtol=1e-3 # linearity in u_r

    # linearity in u_y
    controller, sys, sol1 = solve_with_input(u_y=sin(t), u_r=0)
    controller, sys, sol2 = solve_with_input(u_y=2sin(t), u_r=0)
    @test sum(abs, sol1[controller.ea]) < eps() # This is the acutator model error due to saturation
    @test 2sol1[controller.y] ≈ sol2[controller.y] rtol=1e-3 # linearity in u_y

    # zero error
    controller, sys, sol1 = solve_with_input(u_y=sin(t), u_r=sin(t))
    @test sum(abs, sol1[controller.y]) ≈ 0 atol=sqrt(eps()) 

    # test saturation
    controller, sys, sol1 = solve_with_input(; u_r=10sin(t), u_y=0, 
        controller = PID(; k, Ti, Td, wp, wd=0, Ni, Nd, y_max=10, y_min=-10, name=:controller)
    )
    @test extrema(sol1[controller.y]) == (-10, 10)


    # test P set-point weighting
    controller, sys, sol1 = solve_with_input(; u_r=sin(t), u_y=0, 
        controller = PID(; k, Ti, Td, wp=0, wd, Ni, Nd, y_max, y_min, name=:controller)
    )
    @test sum(abs, sol1[controller.ep]) ≈ 0 atol=sqrt(eps()) 

    # test D set-point weighting
    controller, sys, sol1 = solve_with_input(; u_r=sin(t), u_y=0, 
        controller = PID(; k, Ti, Td, wp, wd=0, Ni, Nd, y_max, y_min, name=:controller)
    )
    @test sum(abs, sol1[controller.ed]) ≈ 0 atol=sqrt(eps()) 


    # zero integral gain
    controller, sys, sol1 = solve_with_input(; u_r=sin(t), u_y=0, 
        controller = PID(; k, Ti=false, Td, wp, wd, Ni, Nd, y_max, y_min, name=:controller)
    )
    @test isapprox(sum(abs, sol1[controller.I.y]), 0, atol=sqrt(eps()))

This issue can remain open until we have reasonable tests for all components where the tests verify all properties of the solution that we are expecting to hold.

[ValentinKaisermayer]

Yeah the PI, PID, LimPI, LimPID tests are very simple for now.

However, the Limiter test is as extensive as I think it can get:

ModelingToolkitStandardLibrary.jl/test/Blocks/nonlinear.jl

Line 45 in e5e6ab7

@test all(isapprox.(sol[lim.output.u], _clamp.(sol[source.output.u], y_min, y_max), atol=1e-2))

[ValentinKaisermayer]

• Extensive tests for the controllers
  • Fail safe test for control error: test if output is equal to reference
  • Test configurations (P, PI, PD, PID)
  • Test set-point weights
  • Test saturation and AWM
• Explicit test for the slew rate

[ValentinKaisermayer]

@baggepinnen can this be closed?
The obsolete tests have to be removed but other than that...

[baggepinnen]

Looks nice, thanks for the effort :)