DelayParentScope variables aren't properly handled

[AayushSabharwal]

So DelayParentScope is a weird case and is giving me a headache. I will be selfish and share this with everyone. When writing collect_scoped_vars! I thought it meant ParentScope nested N times. Looking at the PR that added it, DelayParentScope instead means "behave like LocalScope for N levels, then behave like ParentScope". #3528 fixes this to some extent. However, the fix makes us unable to properly access DelayParentScoped variables via getproperty. Considering this has been very broken for very long (currently getproperty returns an incorrect name for DelayParentScoped variables), I don't think this is a huge deal since we would have had an issue for it long ago. However, it's definitely something to fix.renamespace and namespacing in general would need a non-trivial change to make this happen.

The problem is that our current namespacing behavior relies on identifying which system in the hierarchy a variable "belongs" to. LocalScope means it belongs to the system it is used in. ParentScope means it belongs to the parent of the system it is used in. A system only stores in unknowns or ps the variables that belong to it. DelayParentScope(LocalScope(), N) technically means it belongs to the system N+1 levels above the system it is used in. However, it is namespaced by the bottom N levels in that hierarchy. To do the latter in getproperty, we need to put the variable in the bottom-most system. This is a whole new can of worms, because we don't know what the "bottom-most" system is given just a variable with DelayParentScope(LocalScope(), K). It could be one several levels below this. As a result, when asked the question "does this DelayParentScope belong to the current system, our answer always has to be "yes". For example:

@variables x1(t) x2(t) x3(t) x4(t) x5(t)
x2 = ParentScope(x2)
x3 = ParentScope(ParentScope(x3))
x4 = DelayParentScope(x4)
x5 = GlobalScope(x5)
@parameters p1 p2 p3 p4 p5
p2 = ParentScope(p2)
p3 = ParentScope(ParentScope(p3))
p4 = DelayParentScope(p4)
p5 = GlobalScope(p5)

@named sys1 = ODESystem([D(x1) ~ p1, D(x2) ~ p2, D(x3) ~ p3, D(x4) ~ p4, D(x5) ~ p5], t)
@named sys2 = ODESystem(Equation[], t; systems = [sys1])

This is in our test suite. Hierarchical systems currently work by inspecting the unnamespaced equations of all subsystems, checking if any of the variables belong to the top-level system and then namespacing all the variables of the subsystems. So sys2 looks at D(x4) ~ p4 and asks "does x4 belong to me" - the answer of which is always "yes". So now x4 is an unknown of sys2. Then, sys2 looks at the unknowns of sys1 and namespaces all of them, and gets sys1₊x4 as an unknown. This duplication continuous the higher we go.

My proposal is the following:

• Instead of the unknowns field of a system including only the variables that belong to it, it includes all variables used in the equations of the system (and subsystems).
• These variables have the appropriate SymScope metadata to determine "belongingness".
• When the system is used as a subsystem, the parent just takes all the unknowns and calls renamespace. There is no need to search through the equations again. Since the unknowns have the SymScope metadata, they will be namespaced appropriately.
• In getproperty (getvar) the variables accessible from a system are ones with LocalScope or GlobalScope metadata, or (DelayParentScope metadata and no NAMESPACE_SEPARATOR in their name).

In the above example, sys1 has all of x1...x5 in its unknowns field. Only x1, x4 and x5 are accessible via getproperty. For sys2, it has sys1.x1, x2, x3, sys1.x4 and x5 in the unknowns field. Only sys1.x1, x2 and x5 are accessible via getproperty.

The problem now is what to return when the user calls unknowns (and unknowns_toplevel). My suggestion is to use the above rules for unknowns_toplevel and relax the NAMESPACE_SEPARATOR requirement for unknowns (thereby including DelayParentScoped variables from subsystems).

CC @TorkelE @isaacsas @hersle

[AayushSabharwal]

Marking this as v10 because the change is probably breaking.

[AayushSabharwal]

I'm also more than happy to axe DelayParentScope in v10, make this scoping behavior dispatch-based and if someone asks for it or another weird behavior just tell them to implement it with those dispatches. Granted, this increases our public API surface.

[isaacsas]

I sincerely doubt this feature is used by any Catalyst user. Dropping it in v10 sounds fine to me.

[ChrisRackauckas]

I would think @avinashresearch1 is likely the only one with code that uses it? I think it was just added for some odd thing in the HVAC models.

d = DelayParentScope(d) # skips one level before applying ParentScope
e = DelayParentScope(e, 2) # second argument allows skipping N levels

those two comments in a tutorial are literally its only documentation. A search of all public github code reveals only two cases:

• https://github.com/hstrey/Modeling-SDEs/blob/d29731c1f9669583da0cba6410ae351f13a69a37/ParentScope.jl#L35 deprecated code copied out of MTK
• https://github.com/dfabianus/BioprocessModels.jl/blob/8adbde8a9d917d2ea6947f81977bb323eb47956a/src/BioprocessModels.jl#L21

That last one I think is the same use case as the HVAC model, which is for parameter propagation. Since we now have parameter equations, I think that is a much better way to handle it than this delayed scoping which is inherently hard to reason about.

So yes, I'd say it's fine to just deprecate and remove it in v10 and recommend people using it, almost certainly for shared parameters, uses parameter equations instead.

[AayushSabharwal]

Great. So I'll do something relatively reasonable to make tests pass here and in Catalyst with #3528 and add a depwarn.