fix: remove ambiguities in namespacing of analysis points

Author: AayushSabharwal

src/systems/analysis_points.jl

@@ -314,9 +314,10 @@ function modify_nested_subsystem(
         return fn(root)
     end
     # ignore the name of the root
-    if nameof(root) == hierarchy[1]
-        hierarchy = @view hierarchy[2:end]
+    if nameof(root) != hierarchy[1]
+        error("The name of the root system $(nameof(root)) must be included in the name passed to `modify_nested_subsystem`")
     end
+    hierarchy = @view hierarchy[2:end]
 
     # recursive helper function which does the searching and modification
     function _helper(sys::AbstractSystem, i::Int)
@@ -722,13 +723,14 @@ end
 
 A utility function to get the "canonical" form of a list of analysis points. Always returns
 a list of values. Any value that cannot be turned into an `AnalysisPoint` (i.e. isn't
-already an `AnalysisPoint` or `Symbol`) is simply wrapped in an array.
+already an `AnalysisPoint` or `Symbol`) is simply wrapped in an array. `Symbol` names of
+`AnalysisPoint`s are namespaced with `sys`.
 """
-canonicalize_ap(ap::Symbol) = [AnalysisPoint(ap)]
-canonicalize_ap(ap::AnalysisPoint) = [ap]
-canonicalize_ap(ap) = [ap]
-function canonicalize_ap(aps::Vector)
-    mapreduce(canonicalize_ap, vcat, aps; init = [])
+canonicalize_ap(sys::AbstractSystem, ap::Symbol) = [AnalysisPoint(renamespace(sys, ap))]
+canonicalize_ap(sys::AbstractSystem, ap::AnalysisPoint) = [ap]
+canonicalize_ap(sys::AbstractSystem, ap) = [ap]
+function canonicalize_ap(sys::AbstractSystem, aps::Vector)
+    mapreduce(Base.Fix1(canonicalize_ap, sys), vcat, aps; init = [])
 end
 
 """
@@ -737,7 +739,7 @@ end
 Given a list of analysis points, break the connection for each and set the output to zero.
 """
 function handle_loop_openings(sys::AbstractSystem, aps)
-    for ap in canonicalize_ap(aps)
+    for ap in canonicalize_ap(sys, aps)
         sys, (outvar,) = apply_transformation(Break(ap, true), sys)
         if Symbolics.isarraysymbolic(outvar)
             push!(get_eqs(sys), outvar ~ zeros(size(outvar)))
@@ -776,7 +778,7 @@ All other keyword arguments are forwarded to `linearization_function`.
 function get_linear_analysis_function(
         sys::AbstractSystem, transform, aps; system_modifier = identity, loop_openings = [], kwargs...)
     sys = handle_loop_openings(sys, loop_openings)
-    aps = canonicalize_ap(aps)
+    aps = canonicalize_ap(sys, aps)
     dus = []
     us = []
     for ap in aps
@@ -860,9 +862,7 @@ result of `apply_transformation`.
   with any required further modifications peformed.
 """
 function open_loop(sys, ap::Union{Symbol, AnalysisPoint}; system_modifier = identity)
-    if ap isa Symbol
-        ap = AnalysisPoint(ap)
-    end
+    ap = only(canonicalize_ap(sys, ap))
     tf = LoopTransferTransform(ap)
     sys, vars = apply_transformation(tf, sys)
     return system_modifier(sys), vars
@@ -871,9 +871,9 @@ end
 function linearization_function(sys::AbstractSystem,
         inputs::Union{Symbol, Vector{Symbol}, AnalysisPoint, Vector{AnalysisPoint}},
         outputs; loop_openings = [], system_modifier = identity, kwargs...)
-    loop_openings = Set(map(nameof, canonicalize_ap(loop_openings)))
-    inputs = canonicalize_ap(inputs)
-    outputs = canonicalize_ap(outputs)
+    loop_openings = Set(map(nameof, canonicalize_ap(sys, loop_openings)))
+    inputs = canonicalize_ap(sys, inputs)
+    outputs = canonicalize_ap(sys, outputs)
 
     input_vars = []
     for input in inputs
@@ -897,7 +897,7 @@ function linearization_function(sys::AbstractSystem,
         push!(output_vars, output_var)
     end
 
-    sys = handle_loop_openings(sys, collect(loop_openings))
+    sys = handle_loop_openings(sys, map(AnalysisPoint, collect(loop_openings)))
 
     return linearization_function(system_modifier(sys), input_vars, output_vars; kwargs...)
 end

test/analysis_points.jl

@@ -2,6 +2,7 @@ using ModelingToolkit, ModelingToolkitStandardLibrary.Blocks
 using OrdinaryDiffEq, LinearAlgebra
 using Test
 using ModelingToolkit: t_nounits as t, D_nounits as D, AnalysisPoint, AbstractSystem
+import ModelingToolkit as MTK
 using Symbolics: NAMESPACE_SEPARATOR
 
 @testset "AnalysisPoint is lowered to `connect`" begin
@@ -69,26 +70,21 @@ sys = ODESystem(eqs, t, systems = [P, C], name = :hej)
     @test norm(sol.u[end]) < 1e-6 # This fails without the feedback through C
 end
 
-@testset "get_sensitivity - $name" for (name, sys, ap) in [
+test_cases = [
     ("inner", sys, sys.plant_input),
     ("nested", nested_sys, nested_sys.hej.plant_input),
-    ("inner - nonamespace", sys, :plant_input),
-    ("inner - Symbol", sys, nameof(sys.plant_input)),
-    ("nested - Symbol", nested_sys, nameof(nested_sys.hej.plant_input))
+    ("inner - Symbol", sys, :plant_input),
+    ("nested - Symbol", nested_sys, nameof(sys.plant_input))
 ]
+
+@testset "get_sensitivity - $name" for (name, sys, ap) in test_cases
     matrices, _ = get_sensitivity(sys, ap)
     @test matrices.A[] == -2
     @test matrices.B[] * matrices.C[] == -1 # either one negative
     @test matrices.D[] == 1
 end
 
-@testset "get_comp_sensitivity - $name" for (name, sys, ap) in [
-    ("inner", sys, sys.plant_input),
-    ("nested", nested_sys, nested_sys.hej.plant_input),
-    ("inner - nonamespace", sys, :plant_input),
-    ("inner - Symbol", sys, nameof(sys.plant_input)),
-    ("nested - Symbol", nested_sys, nameof(nested_sys.hej.plant_input))
-]
+@testset "get_comp_sensitivity - $name" for (name, sys, ap) in test_cases
     matrices, _ = get_comp_sensitivity(sys, ap)
     @test matrices.A[] == -2
     @test matrices.B[] * matrices.C[] == 1 # both positive or negative
@@ -104,13 +100,7 @@ S = sensitivity(P, C)      # or feedback(1, P*C)
 T = comp_sensitivity(P, C) # or feedback(P*C)
 =#
 
-@testset "get_looptransfer - $name" for (name, sys, ap) in [
-    ("inner", sys, sys.plant_input),
-    ("nested", nested_sys, nested_sys.hej.plant_input),
-    ("inner - nonamespace", sys, :plant_input),
-    ("inner - Symbol", sys, nameof(sys.plant_input)),
-    ("nested - Symbol", nested_sys, nameof(nested_sys.hej.plant_input))
-]
+@testset "get_looptransfer - $name" for (name, sys, ap) in test_cases
     matrices, _ = get_looptransfer(sys, ap)
     @test matrices.A[] == -1
     @test matrices.B[] * matrices.C[] == -1 # either one negative
@@ -125,13 +115,7 @@ C = -1
 L = P*C
 =#
 
-@testset "open_loop - $name" for (name, sys, ap) in [
-    ("inner", sys, sys.plant_input),
-    ("nested", nested_sys, nested_sys.hej.plant_input),
-    ("inner - nonamespace", sys, :plant_input),
-    ("inner - Symbol", sys, nameof(sys.plant_input)),
-    ("nested - Symbol", nested_sys, nameof(nested_sys.hej.plant_input))
-]
+@testset "open_loop - $name" for (name, sys, ap) in test_cases
     open_sys, (du, u) = open_loop(sys, ap)
     matrices, _ = linearize(open_sys, [du], [u])
     @test matrices.A[] == -1
@@ -146,13 +130,14 @@ eqs = [connect(P.output, :plant_output, C.input)
 sys = ODESystem(eqs, t, systems = [P, C], name = :hej)
 @named nested_sys = ODESystem(Equation[], t; systems = [sys])
 
-@testset "get_sensitivity - $name" for (name, sys, ap) in [
+test_cases = [
     ("inner", sys, sys.plant_input),
     ("nested", nested_sys, nested_sys.hej.plant_input),
-    ("inner - nonamespace", sys, :plant_input),
-    ("inner - Symbol", sys, nameof(sys.plant_input)),
-    ("nested - Symbol", nested_sys, nameof(nested_sys.hej.plant_input))
+    ("inner - Symbol", sys, :plant_input),
+    ("nested - Symbol", nested_sys, nameof(sys.plant_input))
 ]
+
+@testset "get_sensitivity - $name" for (name, sys, ap) in test_cases
     matrices, _ = get_sensitivity(sys, ap)
     @test matrices.A[] == -2
     @test matrices.B[] * matrices.C[] == -1 # either one negative
@@ -163,15 +148,19 @@ end
     ("inner", sys, sys.plant_input, sys.plant_output),
     ("nested", nested_sys, nested_sys.hej.plant_input, nested_sys.hej.plant_output)
 ]
+    inputname = Symbol(join(
+        MTK.namespace_hierarchy(nameof(inputap))[2:end], NAMESPACE_SEPARATOR))
+    outputname = Symbol(join(
+        MTK.namespace_hierarchy(nameof(outputap))[2:end], NAMESPACE_SEPARATOR))
     @testset "input - $(typeof(input)), output - $(typeof(output))" for (input, output) in [
         (inputap, outputap),
-        (nameof(inputap), outputap),
-        (inputap, nameof(outputap)),
-        (nameof(inputap), nameof(outputap)),
+        (inputname, outputap),
+        (inputap, outputname),
+        (inputname, outputname),
         (inputap, [outputap]),
-        (nameof(inputap), [outputap]),
-        (inputap, [nameof(outputap)]),
-        (nameof(inputap), [nameof(outputap)])
+        (inputname, [outputap]),
+        (inputap, [outputname]),
+        (inputname, [outputname])
     ]
         matrices, _ = linearize(sys, input, output)
         # Result should be the same as feedpack(P, 1), i.e., the closed-loop transfer function from plant input to plant output

test/downstream/analysis_points.jl

@@ -64,7 +64,7 @@ import ControlSystemsBase as CS
     prob = ODEProblem(sys, unknowns(sys) .=> 0.0, (0.0, 4.0))
     sol = solve(prob, Rodas5P(), reltol = 1e-6, abstol = 1e-9)
 
-    matrices, ssys = linearize(closed_loop, AnalysisPoint(:r), AnalysisPoint(:y))
+    matrices, ssys = linearize(closed_loop, :r, :y)
     lsys = ss(matrices...) |> sminreal
     @test lsys.nx == 8
 
@@ -129,13 +129,11 @@ end
         t,
         systems = [P_inner, feedback, ref])
 
-    P_not_broken, _ = linearize(sys_inner, AnalysisPoint(:u), AnalysisPoint(:y))
+    P_not_broken, _ = linearize(sys_inner, :u, :y)
     @test P_not_broken.A[] == -2
-    P_broken, _ = linearize(sys_inner, AnalysisPoint(:u), AnalysisPoint(:y),
-        loop_openings = [AnalysisPoint(:u)])
+    P_broken, _ = linearize(sys_inner, :u, :y, loop_openings = [:u])
     @test P_broken.A[] == -1
-    P_broken, _ = linearize(sys_inner, AnalysisPoint(:u), AnalysisPoint(:y),
-        loop_openings = [AnalysisPoint(:y)])
+    P_broken, _ = linearize(sys_inner, :u, :y, loop_openings = [:y])
     @test P_broken.A[] == -1
 
     Sinner = sminreal(ss(get_sensitivity(sys_inner, :u)[1]...))
@@ -154,10 +152,10 @@ end
         t,
         systems = [P_outer, sys_inner])
 
-    Souter = sminreal(ss(get_sensitivity(sys_outer, sys_inner.u)[1]...))
+    Souter = sminreal(ss(get_sensitivity(sys_outer, sys_outer.sys_inner.u)[1]...))
 
     Sinner2 = sminreal(ss(get_sensitivity(
-        sys_outer, sys_inner.u, loop_openings = [:y2])[1]...))
+        sys_outer, sys_outer.sys_inner.u, loop_openings = [:y2])[1]...))
 
     @test Sinner.nx == 1
     @test Sinner == Sinner2
@@ -268,7 +266,7 @@ end
     @test tf(L[2, 1]) ≈ tf(Ps)
 
     matrices, _ = linearize(
-        sys_outer, [sys_outer.inner.plant_input], [sys_inner.plant_output])
+        sys_outer, [sys_outer.inner.plant_input], [nameof(sys_inner.plant_output)])
     G = CS.ss(matrices...) |> sminreal
     @test tf(G) ≈ tf(CS.feedback(Ps, Cs))
 end