-
Notifications
You must be signed in to change notification settings - Fork 93
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 displacement argument to the Lyapunov exponent computation #36
Conversation
Update to the latest version
Update to the latest version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you write state + displacement
you are adding d0
twice
src/lyapunovs.jl
Outdated
@@ -322,7 +321,7 @@ function lyapunov_full(ds::ContinuousDynamicalSystem, T = 10000.0; Ttr = 0.0, | |||
st1 = ds.state | |||
integ1 = ODEIntegrator(ds, T; diff_eq_kwargs=diff_eq_kwargs) | |||
integ1.opts.advance_to_tstop=true | |||
ds.state = st1 .+ d0 | |||
ds.state = st1 .+ displacement(st1, d0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work. You are adding d0
twice.
It is supposed to be state = displacement(state, d0)
src/lyapunovs.jl
Outdated
@@ -254,7 +254,7 @@ function lyapunov(ds::ContinuousDynamicalSystem, T = 10000.0; Ttr = 0.0, | |||
st1 = ds.state | |||
integ1 = ODEIntegrator(ds, T; diff_eq_kwargs=diff_eq_kwargs) | |||
integ1.opts.advance_to_tstop=true | |||
ds.state = st1 .+ d0 | |||
ds.state = st1 .+ displacement(st1, d0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not added yet, please add this new keyword to the documentation string as well.
src/lyapunovs.jl
Outdated
@@ -254,7 +254,7 @@ function lyapunov(ds::ContinuousDynamicalSystem, T = 10000.0; Ttr = 0.0, | |||
st1 = ds.state | |||
integ1 = ODEIntegrator(ds, T; diff_eq_kwargs=diff_eq_kwargs) | |||
integ1.opts.advance_to_tstop=true | |||
ds.state = st1 .+ displacement(st1, d0) | |||
ds.state = displacement.(st1, d0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dot should be at the equals, i.e. .= displacement
Now, the function itself definitely doesn't need a dot. It doesn't operate element-wise on an array, but takes an array and produces a new one.
So ds.state .= displacement(st1, d0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, in many cases the user will specify a function that would not make sense to be broadcasted. This sacrifices some speed but gives flexibility.
Plus this part of the code is not the bottleneck so speed wouldn't matter here.
IMPORTANT. `ContinuousDynamicalSystem` must become parametric type with parameter `{D}`!!!!
the user defined distance may not give exactly `d0` distance
(changes necessary for Lyapunov)
Revert to 42a1f4 and re-add default_rescale
Remove the rest of the `{D}`s
(changes have to be applied to discrete systems as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply rescale
to discrete system lyapunov as well
lyapunov still doesn't work... some problem with the rescale function I guess.
src/lyapunovs.jl
Outdated
T::Real = 10000.0, | ||
return_convergence::Val{B} = Val{false}; | ||
T::Real, | ||
return_convergence::Val{B} = Val{false}(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need the ()
if you do return_convergence::Type{Val{B}}
which is better accepted.
You can know actually use However, now it gives wrong results:
gives 1.0 ... instead of 0.9 ... I dont know whats wrong but it has to be fixed before merging.. |
i was supposed to be one loop outer!!!
Plus I changed all tabs to be 4 spaces instead of 2
plus added tests for discrete systems. The PR is DONE.
BOOOM Test pass this NIGHTMARE can finally be merged after draining my almost empty soul. @SebastianM-C I have changed completely the interface so you might want to study the source code before you use it. |
No description provided.