Fix _varmap_to_vars, re-enable dde.jl test

[MasonProtter]

Closes #2543

cc @AayushSabharwal

It turns out the problem was actually just that we weren't putting symbolic constraints into the values dict, it wasn't about t being missing from the initial u0map (adding that gives the wrong answer actually). I thought I had checked this before but I must have done something else that was breaking alongside it, so this took me way longer to track down than it should have.

This has made it so I can now re-enable the dde.jl test file which was previously excluded from the test suite.

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

Add any other context about the problem here.

[AayushSabharwal]

Why is this necessary? This could fail in cases where a dependent initialization is not fully defined. For example, if x => 2y but y is not specified, val will be 2y when we want it to not be a symbolic quantity.

[MasonProtter]

This just restores the v8 behaviour. In v8 we had

function _varmap_to_vars(varmap::Dict, varlist; defaults = Dict(), check = false,
        toterm = Symbolics.diff2term)
    varmap = merge(defaults, varmap) # prefers the `varmap`
    varmap = Dict(toterm(value(k)) => value(varmap[k]) for k in keys(varmap))
    # resolve symbolic parameter expressions
    for (p, v) in pairs(varmap)
        varmap[p] = fixpoint_sub(v, varmap)
    end

    missingvars = setdiff(varlist, collect(keys(varmap)))
    check && (isempty(missingvars) || throw_missingvars(missingvars))

    out = [varmap[var] for var in varlist]
end

which was changed in v9 to only allow values to have non-symbolic entries, which caused this bug.

e.g. in v8, for the test case linked above, _varmap_to_vars would take in arguments

varmap = Dict{Num, Num}(x₂(t) => 1.0 - 10q1*t, x₀(t) => 1.0 - 10q1*t, x₁(t) => 1.0 - 10q1*t)
varlist = SymbolicUtils.BasicSymbolic{Real}[x₀(t), x₁(t), x₂(t)]
defaults = Dict{Any, Any}(x₂(t) => 1.0 - 10q1*t, d2 => 1, beta0 => 1, v0 => 1, p0 => 0.2, d0 => 5, beta1 => 1, p1 => 0.2, q0 => 0.3, x₁(t) => 1.0 - 10q1*t, q1 => 0.3, d1 => 1, v1 => 1, x₀(t) => 1.0 - 10q1*t)

and compute

out = SymbolicUtils.BasicSymbolic{Real}[1.0 - 3.0t, 1.0 - 3.0t, 1.0 - 3.0t]

whereas in v9 we also have it taking in the identical arguments:

varmap = Dict{Num, Num}(x₂(t) => 1.0 - 10q1*t, x₀(t) => 1.0 - 10q1*t, x₁(t) => 1.0 - 10q1*t)
varlist = SymbolicUtils.BasicSymbolic{Real}[x₀(t), x₁(t), x₂(t)]
defaults = Dict{Any, Any}(x₂(t) => 1.0 - 10q1*t, d2 => 1, beta0 => 1, v0 => 1, p0 => 0.2, d0 => 5, beta1 => 1, p1 => 0.2, q0 => 0.3, x₁(t) => 1.0 - 10q1*t, q1 => 0.3, d1 => 1, v1 => 1, x₀(t) => 1.0 - 10q1*t)

and erroring. This change makes it so that for the same inputs as before, _varmap_to_vars produces the same outputs as before:

out = SymbolicUtils.BasicSymbolic{Real}[1.0 - 3.0t, 1.0 - 3.0t, 1.0 - 3.0t]

It's totally possible there's another way to fix this, but I think what I did is just the minimal change to un-break the change in v9.

[AayushSabharwal]

Please rebase and format so CI runs

[AayushSabharwal]

Those commits shouldn't have appeared here after the rebase 😅

[MasonProtter]

hmm, not sure what went wrong, I just configured rebase = true and pulled from master..

[AayushSabharwal]

Try resetting to before the rebase using git reflog, then

git checkout master
git pull sciml master # assuming sciml is the remote pointing to this repo
git checkout fix-dde
git rebase master
# fix conflicts
git push -f origin fix-dde

usually works for me

[MasonProtter]

Aha, yeah I think I just did a bad order of operations or something, thank you for spelling it out.

[MasonProtter]

So, comparing with #2548, it seems that there's an overall improvement here with a couple of previously failing suites now passing (in addition to the Neuroblox tests which need this change in order to upgrade to MTK v9), and I'm not seeing any new regressions.

[AayushSabharwal]

The failures in #2548 that don't fail here are due to codecov being flaky. This PR does have test regressions in InterfaceI. See:

• https://github.com/SciML/ModelingToolkit.jl/actions/runs/8279178750/job/22653091634?pr=2545#step:6:726
• https://github.com/SciML/ModelingToolkit.jl/actions/runs/8279178750/job/22653091634?pr=2545#step:6:828
• https://github.com/SciML/ModelingToolkit.jl/actions/runs/8279178750/job/22653091634?pr=2545#step:6:1290

[MasonProtter]

ah okay. Yeah, two of those are easy fixes, one is a bit deeper and is related to your concern about the core change here. I'll dig into it today.

[MasonProtter]

So I think that modulo codecov flakiness, this should just leave https://github.com/SciML/ModelingToolkit.jl/actions/runs/8279178750/job/22653091634?pr=2545#step:6:726 failing (unless I just introduced real new problems)

[ChrisRackauckas]

What is the state of this?

[MasonProtter]

Okay, this is finally working and ready to go. The failing downstream test is unrelated, and also present on other current PRs.

[AayushSabharwal]

Just a couple minor comments. Looks good otherwise. Please also rebase this to remove commits such as retrigger CI.

[MasonProtter]

Please also rebase this to remove commits such as retrigger CI.

I've rebased it, but you can also just use the squash and merge button.