Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cumulative kwarg for solving SampledIntegralProblem #182

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sathvikbhagavan
Copy link
Member

No description provided.

@sathvikbhagavan sathvikbhagavan marked this pull request as ready for review September 25, 2023 12:09
@sathvikbhagavan
Copy link
Member Author

@ChrisRackauckas can you review this?

@ChrisRackauckas
Copy link
Member

I guess it's fine but why should this be done in the solver? What is this doing that cumsum after the solver does not?

@sathvikbhagavan
Copy link
Member Author

sathvikbhagavan commented Sep 30, 2023

What is this doing that cumsum after the solver does not?

So, the aim is to get back an integrated time series back from a time series. Basically collecting intermediate results.

src/common.jl Outdated
@@ -114,10 +115,10 @@ function Base.setproperty!(cache::SampledIntegralCache, name::Symbol, x)
end

function SciMLBase.init(prob::SampledIntegralProblem,
alg::SciMLBase.AbstractIntegralAlgorithm;
alg::SciMLBase.AbstractIntegralAlgorithm; cumulative = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that breaks type-stability, it needs to be type level information.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, we should do cumulative::Bool = false to make it type stable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it would need to be a value type, Val(false)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, addressed in 46a11e1

@lxvm
Copy link
Collaborator

lxvm commented Oct 30, 2023

@sathvikbhagavan I read the pr and I'm not sure this actually does what I would expect. Do you actually need the accuracy of a quadrature rule, or would cumsum suffice?

My perspective is that if new quadrature schemes are added for which all the weights change when a single data point is added, then cumulative integrals are equivalent to doing a sampled integral problem for each sub-sequence. If my data represents a time series and I want the best estimate to the integral at each time step, I would solve a sampled integral problem for each sub-sequence of the data, e.g. for all [first(data, n) for n in 1:length(data)]. This is subtly different than cumsum because the quadrature weights used may differ for different numbers of points.

For example, consider the difference between cumsum and applying the trapezoidal rule, which is that in the latter, the first and last data points are given half the weight. When doing a cumulative trapezoidal rule, the cumulative integral would need updates from the last two points in order to be the trapezoidal rule. However, this pr just uses the partial sums of the quadrature rule as the cumulative integrals, which are not themselves quadratures.

@lxvm
Copy link
Collaborator

lxvm commented Mar 2, 2024

@sathvikbhagavan It has been 6 months since you opened this pr so I was wondering if you still need it?

With #222 I'm skeptical that using the partial sums is equivalent to computing the integral of the time series

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants