Open Datseris opened 2 days ago
PeriodicOrbit
struct once the stability is computed.SchmelcherDiakonos
together with PoincareMap
should be included in the docs. A test would be good too.isdiscretetime
shows incorrectly twice in the docs. Docs should show only the docstring of the isdiscretetime(po::PeriodicOrbit)
.isstable
doesn't return true
or false
but instead returns an instance of PeriodicOrbit
. The name of the function should be changed so that it is not misleading.InitialGuess
should have a pretty printing enabled so that the digits after decimal point are rounded to (for example) 5.rootkw
keyword argument of PoincareMap
to minimal_period
.We have a problematic interaction with minimal_period and complete_orbit. minimal_period enforces that the Δt for continuous time is T/100. How can the user control this? In general, why does minimal_period call complete_orbit in the first place? This seems unnecessary. The minimal period has the same points as the "double" or "triple" period. Once we have estimated the minimal T we don't have to recompute/re-complete the orbit. We can just create a PeriodicOrbit type with the new minimal T and reuse its .points field.
User can control it when constructing PeriodicOrbit
but not inside minimal_period
. A keyword argument should be added to minimal_period
as well.
The orbit is recomputed so that .points
field contains the orbit only once and not several times.
The folder structure of src needs to be organized more. We should have one folder for continuous and one for discrete time systems. Inside there we should have subfolders for algorithms that need more than 1 file, otherwise the algorithm as a single file. why is lambdamatrix.jl at the top level, but the Diakons algorithm is inside a folder?
You are right, file lambdamatrix.jl
should not be at top level. It is related to both SchmelcherDiakonos
and DavidchackLai
and so it could be perhaps placed inside top-level of discrete-time systems folder.
right, all sounds good. If you have time to contribute that would be welcomed of course!
Yes, I will look at it in the following days :)
Now that the GSOC is done, I had another review of the whole codebase. Here are my comments. @JonasKoziorek please append the comments you have collected as well.
stable
field, we should usemissing
instead ofnothing
.nothing
is for things that have no known structure.missing
is for things that have known structure, but their actual value is (currently) unknown. Somissing
is possible to know, but not right now. Whilenothing
is in general impossible to know.NonlinearSolve
. Don't doNonlinearSolve.jl
.periodic_orbit
needs to be improved. First, I propose the call signature:periodic_orbit(ds::DynamicalSystem, alg::PeriodicOrbitFinder[, ig])
Then, say that it returns aPeriodicOrbit
instance, and@ref
the docstring. Then, say thatig
is an instance of anInitialGuess
and@ref
it. If not given, it is derived fromds
.periodic_orbits
. Say the return type in the text, because the call signature is too long. Removeigs
from call signature like above. Also say why this function exists: because some algorithms find multiple POs in one go.periodic_orbits
should be first. Additionally, the algorithms should be in the same page as the Public API, not in a dedicated page.src
needs to be organized more. We should have one folder for continuous and one for discrete time systems. Inside there we should have subfolders for algorithms that need more than 1 file, otherwise the algorithm as a single file. why islambdamatrix.jl
at the top level, but the Diakons algorithm is inside a folder?minimal_period
andcomplete_orbit
.minimal_period
enforces that the Δt for continuous time isT/100
. How can the user control this? In general, why doesminimal_period
callcomplete_orbit
in the first place? This seems unnecessary. The minimal period has the same points as the "double" or "triple" period. Once we have estimated the minimalT
we don't have to recompute/re-complete the orbit. We can just create aPeriodicOrbit
type with the new minimalT
and reuse its.points
field._periodic_orbits!
function here does not have any type annotations for dispatch, but is really only valid for the schmelher diakonos algorithm. It should be renamed to_periodic_orbits_sd!
or something.igs
is unecessary: https://github.com/JuliaDynamics/PeriodicOrbits.jl/blob/13c53d0248d8116fd54254bc45067382d1ad74eb/src/algorithms/schmelcher_diakonos.jl#L66 simply iterate over the existingigs
and useig.u0
as the initial condition.periodic_orbits
function should do a cleanup/filtering: only return unique periodic orbits. This should also be mentioned in its docstring.PeriodicOrbit(ds::DynamicalSystem, ...)
constructors,Δt, stable
should be keyword arguments, so that the call signature is the same irrespectively of dynamical system. The discrete time case also getsΔt = 1
as keyword argument that is otherwise ignored.