test: fix DelayParentScope test

[AayushSabharwal]

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.

[isaacsas]

@AayushSabharwal are the failures here because the update to MTK hasn't been released yet?

[AayushSabharwal]

Yes, and also because it needs a couple more tweaks to account for the new Initial parameters MTK adds.

[isaacsas]

@AayushSabharwal the test that is failing is one where the initial condition is Float32 but it is now getting promoted to Float64 in the solution object?

(That one was passing just yesterday.)

[TorkelE]

There is an error now where type specification of solution objects gets dropped:

    # Create model. Checks when input type is `Float64` the produced values are also `Float64`.
    rn = @reaction_network begin
        (k1,k2), X1 <--> X2
    end

    # Checks when values are `Float32` (a valid type and should be preserved).
    u0 = [:X1 => 1.0f0, :X2 => 3.0f0]
    ps = [:k1 => 2.0f0, :k2 => 3.0f0]
    oprob = ODEProblem(rn, u0, 1.0f0, ps)
    osol = solve(oprob, Tsit5())
    @test eltype(osol[:X1]) == eltype(osol[:X2]) == typeof(oprob[:X1]) == typeof(oprob[:X2]) == Float32 # These are all `Float64`
    @test eltype(osol.t) == typeof(oprob.tspan[1]) == typeof(oprob.tspan[2]) == Float32

[isaacsas]

Can we set this Float32 test to test_broken for now? I'm fine making it a blocker for release, but it would be nice to get CI working for other PRs.

[TorkelE]

There i somtehing in the Jacobian tests again (again relating to sparse Jacobians, SciML/ModelingToolkit.jl#3554). I have disabledt hem for now so that we can continue checking if there is any more problems.

[TorkelE]

Ok, seems we are not too far off. Think the only stuff causing failures not fixed here is
SciML/ModelingToolkit.jl#3553
SciML/ModelingToolkit.jl#3554

[isaacsas]

Is the CI failure just from testing floating point results are equal instead of agreeing to some tolerance? If so just update the test to test agreement to some reasonable tolerance. We generally shouldn’t test floating point results are exactly equal anyways.

[TorkelE]

So I think doing approx is a sensible solution. There is a discussion over at SciML/ModelingToolkit.jl#3554 (comment) and there might be some reason why the MTK people would want to have a look. From our purposes, using approx should be more than fine.

[ChrisRackauckas]

Yeah with how SciML/ModelingToolkit.jl#3554 was narrowed down, I thought at first it could be something a bit more fundamental but it was narrowed down to CSE, which is an expression reordering to save computations thing. For reference, common subexpression elimination is

x = (x + 1) * y + (x + 1) * z

you instead do

tmp = x + 1
x = tmp*y + tmp*z

Symbolics.jl build function now supports doing this, and MTK turns it on by default. You can see it changes the BCR sparse Jacobian build from 200 seconds to 20 seconds:

───────────────────────────────────────────────────────────────────────────
───────────────────────────────────
                                                                     Time  
                  Allocations      
                                                            ───────────────
────────   ────────────────────────
                     Tot / % measured:                            500s /  9
3.2%           34.4GiB /  90.1%    

Section                                             ncalls     time    %tot
     avg     alloc    %tot      avg
───────────────────────────────────────────────────────────────────────────
───────────────────────────────────
Compile jacobian - no CSE                                1     213s   45.7%
    213s   3.05GiB    9.8%  3.05GiB
Compile jacobian - CSE                                   1    77.8s   16.7%
   77.8s   0.96GiB    3.1%  0.96GiB
Calculate jacobian - without hashconsing                 1    73.3s   15.7%
   73.3s   10.5GiB   34.0%  10.5GiB
Calculate jacobian - hashconsing, without caching        1    72.1s   15.5%
   72.1s   10.5GiB   33.8%  10.5GiB
Calculate jacobian - hashconsing and caching             1    23.7s    5.1%
   23.7s   5.19GiB   16.8%  5.19GiB
Build jacobian - no CSE                                  1    4.42s    0.9%
   4.42s    615MiB    1.9%   615MiB
Build jacobian - CSE                                     1    1.65s    0.4%
   1.65s    162MiB    0.5%   162MiB
Compute jacobian - no CSE                                1   91.4μs    0.0%
  91.4μs      272B    0.0%     272B
Compute jacobian - CSE                                   1   46.6μs    0.0%
  46.6μs      272B    0.0%     272B
───────────────────────────────────────────────────────────────────────────
───────────────────────────────────

in the unreleased benchmarks SciML/SciMLBenchmarks.jl#1178

But yes, it is a floating point level difference and I don't think Catalyst tests should care about a 1e-16 from a reordering of arithmetic, but at the same time I do want to follow up with this in MTK just to find out why the in-place and out of place are not giving exactly the same ordering post LLVM optimizations.

[isaacsas]

I had seen the recent CSE stuff and figured that was the cause, but yes we shouldn’t generally expect floating point equality except for results from the exact same function. I figured it could be that something was slightly different in the codegen for in and out of place.

[isaacsas]

Use a smaller tolerance. Seems like they should agree to basically floating point precision (so maybe check agreement to one or two lower orders)?

[TorkelE]

will do, think there might be some other cases where his happens as well. When I do I will add like 1e-14 to them and we should be good.

[isaacsas]

Does this one need a tolerance update to be a bit more strict (like the others)?

[TorkelE]

agree, this one should have as well

[TorkelE]

ok, test passes now

[isaacsas]

@TorkelE one comment on a test where you missed using a tighter tolerance, but fine to then merge when you are happy.

[TorkelE]

Updated. When tests pass, are we happy to merge?

[TorkelE]

Done, thanks for the help @AayushSabharwal !