Closed oscardssmith closed 8 months ago
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 :warning: |
src/problems/problem_interface.jl | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
tests added. I think this is ready to merge.
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?
I guess the is_observed
method can also be removed for AbstractSciMLProblem
in problems/problem_interface.jl
as well.
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)
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 :laughing:
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
SII.observed
falls back to the definition in SymolicIndexingInterfase
(specifically observed(sys, sym) = observed(symbolic_container(sys), sym)
.
lets see how CI likes this.
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
it gets implimented by SciMLFunction
.
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.
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?
Oh, I think the Optimization problem is https://github.com/SciML/SciMLBase.jl/pull/600
rebased. Let's try this again.
@Vaibhavdixit02 I thought the Optimization stats merging was handled?
It is, this looks like something else iiuc. The downstream CI right?
oh, I must've not refreshed. It's an @AayushSabharwal issue then 😅
Yeah this is a bad test I fixed in https://github.com/SciML/SciMLBase.jl/pull/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])
which PR?
Bump. This is kinda needed for MTK
agreed. I'm unsure why this pr would have regressed inference though.
Could you try rebasing this?
rebased.
Downstream still failing.
yes. I still have no idea why this PR causes us to lose inference precision.
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.
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.
Something feels deeply wrong to me here. Should sdeprob.f.observed(sym)
just work? where is the ODE version done ( odeprob.f.observed(sym)
)?
that seems to have made things much worse.
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.
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.
Succeeded by https://github.com/SciML/SciMLBase.jl/pull/615
I can't close this PR
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.