-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
Strange behavior of save_at=false #1842
Strange behavior of save_at=false #1842
Comments
I also encountered this issue today. When solving an
An example is given by the following code. function f(du, u, p, t)
du[1] = -cos(u[1]) * u[1]
end
prob = ODEProblem(f, [10], (0.0, 0.4))
vcat(solve(prob, Tsit5(); saveat = 0:.1:.4).t...)
# => [0.0; 0.1; 0.2; 0.3; 0.4]
vcat(solve(prob, Tsit5(); saveat = 0:.1:.4, save_start = true, save_end = true).t...)
# => [0.0; 0.1; 0.2; 0.3; 0.4]
vcat(solve(prob, Tsit5(); saveat = 0:.1:.4, save_start = false, save_end = false).t...)
# => [0.1; 0.2; 0.3; 0.4]
This apparent inconsistency between how If it is concluded that this is indeed a bug, I would propose the following solution (and can take a stab at implementing it).
If the example above is the intended behaviour, it might be worthwhile to document this very explicitly (unless it already is and I missed it). Thanks! |
this seems relatively bugish. IMO, the behavior should probably be that saveat is respected. if I were writing the API from scratch, I would probably delete save_start and save_end and just make the default saveat be the tspan, but that's obviously breaking to do now |
That would be insufficient for many applications. It's running under the assumption that there's a an easy static representation for any array which just isn't true. If you try to represent the
The booleans are explicit overrides. It's assumed that if you're going to set those booleans then it's going to be respected. IIRC that is the documented behavior, "regardless of the other saving settings", precisely for the reasons mentioned above.
That one is a bug. |
wouldn't that example just be |
Nowadays mostly yes, but since ranges have interesting higher precision corrections to stepping points they are not compatible with most GPU kernels. On CUDA that now has a fallback to the inexact form, and that won't work on many platforms like Metal still. |
can't you collect the saveat points in advance? you need all of them in the solution object anyway. |
Not if you want it to be both static and different between solves (can be different sized) |
Sure, that's fair too, as long as it's consistent!
Interesting, this is the exact opposite of how I interpreted that sentence 😅 When I read "Denotes whether the final timepoint is forced to be saved, regardless of the other saving settings.", I think "When this option is My confusion probably stems from the fact that it's a negative explicit override: the default is Note that |
I'd be happy to take a PR to improve the docstring clarity. That might be the start here, and since I am the one who wrote it and knows what it should mean, I'm the worst person to edit it since now you know what is unclear about it 😅 . So please PR. |
Do you want a PR that documents the current behaviour, or first to make the behaviour more consistent? And if so, in what direction? |
I tried a few more examples in addition to the ones from my previous comment. Combined: vcat(solve(prob, Tsit5(); saveat = 0:.1:.4).t...)
# => [0.0; 0.1; 0.2; 0.3; 0.4]
vcat(solve(prob, Tsit5(); saveat = 0:.1:.4, save_start = true, save_end = true).t...)
# => [0.0; 0.1; 0.2; 0.3; 0.4]
vcat(solve(prob, Tsit5(); saveat = 0:.1:.4, save_start = false, save_end = false).t...)
# => [0.1; 0.2; 0.3; 0.4]
vcat(solve(prob, Tsit5()).t...)
# => [0.0; 0.07; 0.16; 0.24; 0.34; 0.4]
vcat(solve(prob, Tsit5(); save_start = true, save_end = true).t...)
# => [0.0; 0.07; 0.16; 0.24; 0.34; 0.4]
vcat(solve(prob, Tsit5(); save_start = false, save_end = false).t...)
# => [0.07; 0.16; 0.24; 0.34; 0.4]
vcat(solve(prob, Tsit5(); saveat = [.2]).t...)
# => [0.2]
vcat(solve(prob, Tsit5(); saveat = [.2], save_start = true, save_end = true).t...)
# => [0.0; 0.2; 0.4]
vcat(solve(prob, Tsit5(); saveat = [.2], save_start = false, save_end = false).t...)
# => [0.2] Replacing How I understand the current behaviour is as follows:
I have created two pull requests (here and here) to add this wording to the documentation. I do feel that these options are overly complicated in their behaviour and should currently be avoided by users... |
Let's centralise discussion here. This comment mentions that a bugfix is necessary, rather than a doc change. Should both arguments be handled as save_start currently is; that is, overriding saveat when explicitly given? If so, I imagine there are a lot of different solvers implementing the DiffEq.jl interface that will need to be changed (or, of course, that are currently inconsistent with |
We should create a central discussion on DifferentialEquations.jl for this.
Yes.
yes, we should make an issue with checkboxes so we do them all. DiffEqGPU, Sundials, ODEInterface, DASSL, LSODA, MATLABDiffEq, SciPyDiffEq, etc. off the top of my head. It's in the docs page. We can mark this as good first issue since #2095 shows how to do it (most of the packages have a similar |
Looks good! Due to the current defaults, the following potentially unintuitive situation remains: vcat(solve(prob, Tsit5(); saveat = 0:.15:.4).t...)
# => [0.0; 0.15; 0.3]
vcat(solve(prob, Tsit5(); saveat = .15).t...)
# => [0.0; 0.15; 0.3; 0.4] I don't know if you consider this intended behaviour, or not. |
Yeah it's a bit odd but it's intended. I would say what's odd is that |
I am getting strange behavior with the different versions of save_at and save_start and save_end. Here is a MWE:
I expect the solution at the two times 0.2 and 1.3 to be printed. This is what happens. Next, I explicitly add the start and end times to
saveat
, and specify that save_start and save_end are false. I expect the solution at four times values to be printed. However, only three are printed.This might be a missing edge case.
The text was updated successfully, but these errors were encountered: