fix observed for DEintegrator

[oscardssmith]

This still needs a test, but it turns out both of these methods are unnecessary (since they just fall back to working on the ScimlFunction) and the observed implementation here is just incorrect.

[codecov]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (0c62711) 26.25% compared to head (1cb0ec2) 30.53%.
Report is 4 commits behind head on master.

Files	Patch %	Lines
src/scimlfunctions.jl	0.00%	9 Missing ⚠️
src/problems/problem_interface.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage   26.25%   30.53%   +4.27%     
==========================================
  Files          54       54              
  Lines        4113     4120       +7     
==========================================
+ Hits         1080     1258     +178     
+ Misses       3033     2862     -171     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[oscardssmith]

tests added. I think this is ready to merge.

[AayushSabharwal]

I get the is_observed fallback, but what is wrong with the observed implementation?

Without it observed expressions won't work for integrators. i.e. getu(integrator, sys.x + sys.y)(integrator) would error?

[AayushSabharwal]

I guess the is_observed method can also be removed for AbstractSciMLProblem in problems/problem_interface.jl as well.

[oscardssmith]

the correct observed function would just call getobserved. This version both gets the observed function and then calls it with work incorrect arguments (it doesn't pass in u, p, or t)

[oscardssmith]

I guess the is_observed method can also be removed for AbstractSciMLProblem in problems/problem_interface.jl as well.

good call on this. When removing it, I discovered that the observed function was broken in the same way as the integrator one 😆

[AayushSabharwal]

the correct observed function would just call getobserved. This version both gets the observed function and then calls it with work incorrect arguments (it doesn't pass in u, p, or t)

Where is this function defined? There's no method that SII.observed will fall back to now.

Also you'll need to disambiguate between ModelingToolkit.observed and SII.observed in the test since they're both loaded

[oscardssmith]

SII.observed falls back to the definition in SymolicIndexingInterfase (specifically observed(sys, sym) = observed(symbolic_container(sys), sym).

[oscardssmith]

lets see how CI likes this.

[AayushSabharwal]

SII.observed falls back to the definition in SymolicIndexingInterfase (specifically observed(sys, sym) = observed(symbolic_container(sys), sym).

Yeah but eventually something needs to implement it, because otherwise it'll fallback to the system which doesn't

[oscardssmith]

it gets implimented by SciMLFunction.

[AayushSabharwal]

I only just realized that SciMLFunction already implements this. I was waiting for the methods to be added 😅 Sorry for the delay. Could you make sure the symbolic indexing tests pass before merging?

[oscardssmith]

I believe the symbolic indexing tests are passing (they run locally at least), but I'm having some trouble figuring out which of the CI runs is the related one.

[AayushSabharwal]

It's CI / test (Downstream, 1) which seems to fail before it even gets to that point. The error seems to be an Optimization.jl thing?

[oscardssmith]

Oh, I think the Optimization problem is #600

[oscardssmith]

rebased. Let's try this again.

[ChrisRackauckas]

@Vaibhavdixit02 I thought the Optimization stats merging was handled?

[Vaibhavdixit02]

It is, this looks like something else iiuc. The downstream CI right?

[ChrisRackauckas]

oh, I must've not refreshed. It's an @AayushSabharwal issue then 😅

[AayushSabharwal]

Yeah this is a bad test I fixed in #584

get_obs = getu(sys_simplified, lorenz1.x + lorenz2.x)
get_obs_arr = getu(sys_simplified, [lorenz1.x + lorenz2.x, lorenz1.y + lorenz2.y])

should be

get_obs = getu(prob, lorenz1.x + lorenz2.x)
get_obs_arr = getu(prob, [lorenz1.x + lorenz2.x, lorenz1.y + lorenz2.y])

[ChrisRackauckas]

which PR?

[AayushSabharwal]

#584

[AayushSabharwal]

Bump. This is kinda needed for MTK

[ChrisRackauckas]

This looked related: https://github.com/SciML/SciMLBase.jl/actions/runs/7649534210/job/20844091535?pr=595#step:6:2460 ?

[oscardssmith]

agreed. I'm unsure why this pr would have regressed inference though.

[AayushSabharwal]

Could you try rebasing this?

[oscardssmith]

rebased.

[ChrisRackauckas]

Downstream still failing.

[oscardssmith]

yes. I still have no idea why this PR causes us to lose inference precision.

[AayushSabharwal]

Changing the is_observed function in scimlfunctions.jl to

SymbolicIndexingInterface.is_observed(fn::AbstractSciMLFunction, sym) = has_sys(fn) ? is_observed(fn.sys, sym) : has_observed(fn)

helps, but the same error occurs later on.

[AayushSabharwal]

Changing SII.observed in the same file to:

function SymbolicIndexingInterface.observed(fn::AbstractSciMLFunction, sym)
  if has_observed(fn)
    if is_time_dependent(fn)
      return if hasmethod(fn.observed, Tuple{typeof(sym)})
        fn.observed(sym)
      else
        let obs = fn.observed, sym = sym
          (u, p, t) -> obs(sym, u, p, t)
        end
      end
    else
      return if hasmethod(fn.observed, Tuple{typeof(sym)})
        fn.observed(sym)
      else
        let obs = fn.observed, sym = sym
          (u, p) -> obs(sym, u, p)
        end
      end
    end
  end
  error("SciMLFunction does not have observed")
end

Fixes this. I'm not completely sure why. We need the if condition because the generated observed for SDEs doesn't allow calling it with only a symbol (sdeprob.f.observed(sym) throws a MethodError). hasmethod is static so the branches should be removed at compile time.

[oscardssmith]

Something feels deeply wrong to me here. Should sdeprob.f.observed(sym) just work? where is the ODE version done ( odeprob.f.observed(sym))?

[oscardssmith]

that seems to have made things much worse.

[AayushSabharwal]

Should sdeprob.f.observed(sym) just work? where is the ODE version done ( odeprob.f.observed(sym))?

Yeah it should work. All of this is implemented in MTK, and the SDE version just isn't up to spec.

that seems to have made things much worse.

Yeah, that's weird.

[oscardssmith]

since our timezones match quite poorly, would you mind taking this over (e.g. possibly making your own branch so you can push without waiting for me?) it would be very nice to get this merged soon.

[AayushSabharwal]

Succeeded by #615

I can't close this PR