feat: add discrete saving feature to ODESolution #645
Conversation
src/solutions/solution_interface.jl
Outdated
| return ps | ||
| end | ||
| t = A.t[tidx] | ||
| idx = searchsortedfirst(discretes.t, t) |
There was a problem hiding this comment.
searchsortedfirst returns the first index greater than or equivalent to t, that means that the case where there is an exact match t has to be handled separately, since then you should return discretes.u[idx] instead of discretes.u[idx - 1]?
There was a problem hiding this comment.
Thanks for pointing that out! I'll fix it
src/solutions/solution_interface.jl
Outdated
| t = A.t[tidx] | ||
| idx = searchsortedfirst(discretes.t, t) | ||
| if idx == firstindex(discretes.t) | ||
| error("This should never happen: there is no discrete parameter value before the current time") |
There was a problem hiding this comment.
the same for this error, I should be able to get the value exactly at the first index.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #645 +/- ##
==========================================
- Coverage 41.53% 0.00% -41.54%
==========================================
Files 55 55
Lines 4582 4591 +9
==========================================
- Hits 1903 0 -1903
- Misses 2679 4591 +1912 ☔ View full report in Codecov by Sentry. |
AayushSabharwal
force-pushed
the
as/discrete-save
branch
2 times, most recently
from
7df7619
to
7f6658d
Compare
AayushSabharwal
force-pushed
the
as/discrete-save
branch
from
a40a2a3
to
7f57ef0
Compare
AayushSabharwal
force-pushed
the
as/discrete-save
branch
from
7f57ef0
to
1e62b87
Compare
|
Requires SciML/RecursiveArrayTools.jl#372 to merge and SciML/DiffEqBase.jl#1019 to work |
AayushSabharwal
force-pushed
the
as/discrete-save
branch
from
2eb3a80
to
b509d04
Compare
| ps = parameter_values(sol) | ||
| # NOTE: This is basically SII.parameter_values_at_time but that isn't public API | ||
| # and once we move interpolation to SII, there's no reason for it to be | ||
| if is_parameter_timeseries(sol) == Timeseries() |
There was a problem hiding this comment.
I had expected a function with the name is_parameter_timeseries to return a boolean, answering the question of the function name.
There was a problem hiding this comment.
Yeah, it's actually a trait function necessary for dispatching which is why it returns a singleton object
There was a problem hiding this comment.
In that case, parameter_timeseries_trait would perhaps be better?
A function starting with is but not returning a bool is very confusing
There was a problem hiding this comment.
Yeah that work work. I initially chose is_parameter_timeseries because we already have is_timeseries which (unfortunately) is also a trait function for the IsTimeseriesTrait, which is_parameter_timeseries also leveranges.
src/solutions/ode_solutions.jl
Outdated
| masked_discs = map(sol.discretes) do disc | ||
| mini = searchsortedfirst(disc.t, mint) | ||
| maxi = searchsortedlast(disc.t, maxt) | ||
| disc[mini:maxi] |
There was a problem hiding this comment.
this implementation is only correct if I is a unit range? Should the argument type of I be restricted so that this method cannot be called with, say, I = [1, 2, 4]?
There was a problem hiding this comment.
Fixed in bb65e2d
AayushSabharwal
force-pushed
the
as/discrete-save
branch
2 times, most recently
from
ab153bd
to
ea9d214
Compare
AayushSabharwal
force-pushed
the
as/discrete-save
branch
from
ea9d214
to
0de0ce6
Compare
AayushSabharwal
force-pushed
the
as/discrete-save
branch
2 times, most recently
from
95682ad
to
1afed50
Compare
AayushSabharwal
force-pushed
the
as/discrete-save
branch
from
a20733b
to
5b7e92b
Compare
Co-authored-by: Fredrik Bagge Carlson <[email protected]>
AayushSabharwal
force-pushed
the
as/discrete-save
branch
from
5b7e92b
to
15e6fc2
Compare
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.