feat: add discrete saving feature to ODESolution #645

AayushSabharwal · 2024-03-06T14:36:30Z

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.

baggepinnen · 2024-03-07T06:43:41Z

src/solutions/solution_interface.jl

+        return ps
+    end
+    t = A.t[tidx]
+    idx = searchsortedfirst(discretes.t, t)


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]?

Thanks for pointing that out! I'll fix it

Fixed in 1533e0f

baggepinnen · 2024-03-07T06:44:11Z

src/solutions/solution_interface.jl

+    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")


the same for this error, I should be able to get the value exactly at the first index.

Fixed in 1533e0f

codecov · 2024-03-07T11:27:46Z

Codecov Report

Attention: Patch coverage is 0% with 116 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (9185795) to head (ea9d214).

Files	Patch %	Lines
src/solutions/ode_solutions.jl	0.00%	104 Missing ⚠️
ext/SciMLBaseZygoteExt.jl	0.00%	8 Missing ⚠️
src/integrator_interface.jl	0.00%	2 Missing ⚠️
src/solutions/solution_interface.jl	0.00%	2 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

AayushSabharwal · 2024-05-03T14:48:28Z

Requires SciML/RecursiveArrayTools.jl#372 to merge and SciML/DiffEqBase.jl#1019 to work

baggepinnen · 2024-05-23T05:36:04Z

src/solutions/ode_solutions.jl

+    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()


I had expected a function with the name is_parameter_timeseries to return a boolean, answering the question of the function name.

Yeah, it's actually a trait function necessary for dispatching which is why it returns a singleton object

In that case, parameter_timeseries_trait would perhaps be better?

A function starting with is but not returning a bool is very confusing

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.

baggepinnen · 2024-05-23T05:40:54Z

src/solutions/ode_solutions.jl

+        masked_discs = map(sol.discretes) do disc
+            mini = searchsortedfirst(disc.t, mint)
+            maxi = searchsortedlast(disc.t, maxt)
+            disc[mini:maxi]


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]?

Fixed in bb65e2d

Co-authored-by: Fredrik Bagge Carlson <[email protected]>

baggepinnen reviewed Mar 7, 2024

View reviewed changes

AayushSabharwal force-pushed the as/discrete-save branch 2 times, most recently from 7df7619 to 7f6658d Compare March 11, 2024 08:02

AayushSabharwal requested a review from baggepinnen March 11, 2024 08:03

AayushSabharwal mentioned this pull request Mar 11, 2024

feat: add discrete saving functionality SciML/DiffEqBase.jl#1019

Closed

5 tasks

AayushSabharwal force-pushed the as/discrete-save branch from a40a2a3 to 7f57ef0 Compare March 11, 2024 10:13

AayushSabharwal marked this pull request as ready for review March 12, 2024 05:10

AayushSabharwal force-pushed the as/discrete-save branch from 7f57ef0 to 1e62b87 Compare May 3, 2024 14:47

AayushSabharwal marked this pull request as draft May 3, 2024 14:47

AayushSabharwal mentioned this pull request May 21, 2024

feat: implement new discrete saving functionality SciML/ModelingToolkit.jl#2728

Merged

5 tasks

AayushSabharwal force-pushed the as/discrete-save branch from 2eb3a80 to b509d04 Compare May 21, 2024 15:01

AayushSabharwal mentioned this pull request May 21, 2024

fix: don't depend on ODESolution internals SciML/BoundaryValueDiffEq.jl#186

Merged

5 tasks

baggepinnen reviewed May 23, 2024

View reviewed changes

AayushSabharwal force-pushed the as/discrete-save branch 2 times, most recently from ab153bd to ea9d214 Compare June 4, 2024 09:25

AayushSabharwal force-pushed the as/discrete-save branch from ea9d214 to 0de0ce6 Compare June 17, 2024 08:50

AayushSabharwal marked this pull request as ready for review June 17, 2024 08:50

AayushSabharwal marked this pull request as draft June 18, 2024 05:55

AayushSabharwal force-pushed the as/discrete-save branch 2 times, most recently from 95682ad to 1afed50 Compare July 2, 2024 07:16

AayushSabharwal force-pushed the as/discrete-save branch 2 times, most recently from 5b7e92b to 15e6fc2 Compare July 7, 2024 20:59

AayushSabharwal force-pushed the as/discrete-save branch 2 times, most recently from 28db596 to a9c937f Compare July 23, 2024 06:37

AayushSabharwal marked this pull request as ready for review July 23, 2024 10:37

AayushSabharwal force-pushed the as/discrete-save branch from a9c937f to ef94a20 Compare July 23, 2024 10:37

AayushSabharwal added 2 commits July 23, 2024 23:28

feat: implement new SII discrete saving interface

cea0536

fix: fix ODESolution-related adjoints

854e4fd

AayushSabharwal and others added 8 commits July 23, 2024 23:28

feat: move clock interface here

5de5d1b

refactor: refactor interpolation with indexed clocks

51f554b

test: rewrite hybrid system tests to not use MTK

8af217d

feat: add clock phase

20efc75

Co-authored-by: Fredrik Bagge Carlson <[email protected]>

feat: add discrete variables support to solution plot recipe

f769206

build: bump RAT and SII compat

cfe63d7

fix: remove ambiguity in _updated_u0_p_internal

d71ec13

test: reduce ambiguities threshold

fe68aaa

AayushSabharwal force-pushed the as/discrete-save branch from ef94a20 to fe68aaa Compare July 23, 2024 17:59

ChrisRackauckas approved these changes Jul 23, 2024

View reviewed changes

ChrisRackauckas merged commit d7edbe7 into SciML:master Jul 23, 2024
33 of 41 checks passed

AayushSabharwal deleted the as/discrete-save branch July 24, 2024 05:09

JoshuaLampert mentioned this pull request Jul 24, 2024

implement get_tmp_cache SKopecz/PositiveIntegrators.jl#113

Merged

ranocha mentioned this pull request Jul 24, 2024

v2.45 broke plotting with idxs as a tuple with a function #746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add discrete saving feature to ODESolution #645

feat: add discrete saving feature to ODESolution #645

AayushSabharwal commented Mar 6, 2024

baggepinnen Mar 7, 2024

AayushSabharwal Mar 7, 2024

AayushSabharwal Mar 7, 2024

baggepinnen Mar 7, 2024

AayushSabharwal Mar 7, 2024

codecov bot commented Mar 7, 2024 •

edited

Loading

AayushSabharwal commented May 3, 2024

baggepinnen May 23, 2024

AayushSabharwal May 23, 2024

baggepinnen May 23, 2024

AayushSabharwal May 23, 2024 •

edited

Loading

baggepinnen May 23, 2024

AayushSabharwal May 23, 2024

feat: add discrete saving feature to ODESolution #645

feat: add discrete saving feature to ODESolution #645

Conversation

AayushSabharwal commented Mar 6, 2024

Checklist

Additional context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2024 • edited Loading

Codecov Report

AayushSabharwal commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AayushSabharwal May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2024 •

edited

Loading

AayushSabharwal May 23, 2024 •

edited

Loading