Review comments on the codebase after GSOC

[Datseris]

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.

• In the README say "throughout the docs 'Periodic Orbit' is sometimes condensed to PO in the documentation."
• For the stable field, we should use missing instead of nothing. nothing is for things that have no known structure. missing is for things that have known structure, but their actual value is (currently) unknown. So missing is possible to know, but not right now. While nothing is in general impossible to know.
• When referring to a Julia package, either use .jl without monotype, such as NonlinearSolve.jl, or use module name without .jl and monotype, such as NonlinearSolve. Don't do NonlinearSolve.jl.
• The docstring of periodic_orbit needs to be improved. First, I propose the call signature: periodic_orbit(ds::DynamicalSystem, alg::PeriodicOrbitFinder[, ig])
  Then, say that it returns a PeriodicOrbit instance, and @ref the docstring.
  Then, say that ig is an instance of an InitialGuess and @ref it. If not given, it is derived from ds.
• Similar change for periodic_orbits. Say the return type in the text,
  because the call signature is too long. Remove igs from call signature like above. Also say why this function exists: because some algorithms find multiple POs in one go.
• In the docs the Public API needs to be re-arranged. periodic_orbits should be first. Additionally, the algorithms should be in the same page as the Public API, not in a dedicated page.
• 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?
• Are you also porting the DavidLaiHai algorithm?
• 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.
• This line is problematic:

  PeriodicOrbits.jl/src/algorithms/schmelcher_diakonos.jl

  Line 65 in 13c53d0

  function _periodic_orbits!(POs, ds, alg, igs, Λ)

  the generic _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.
• Creating this extra igs is unecessary:

  PeriodicOrbits.jl/src/algorithms/schmelcher_diakonos.jl

  Line 66 in 13c53d0

  igs = [ig.u0 for ig in igs]

  simply iterate over the existing igs and use ig.u0 as the initial condition.
• This function:

  PeriodicOrbits.jl/src/algorithms/optimized_shooting.jl

  Lines 90 to 99 in 13c53d0

  	function periodic_orbits(ds::CoupledODEs, alg::OptimizedShooting, igs::Vector{<:InitialGuess})
  	pos = []
  	for ig in igs
  	res = periodic_orbit(ds, alg, ig)
  	if !isnothing(res)
  	push!(pos, res)
  	end
  	end
  	return pos
  	end

  should be a generic fallback method. It should be implemented once at the API file, and not specified for one algorithm. It works for all algorithms the same way with this simple loop. Only algorithms that can actually utilize this function do so and extend it by adding new methods. Furthermore, at its end, the periodic_orbits function should do a cleanup/filtering: only return unique periodic orbits. This should also be mentioned in its docstring.
• For the 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.
• A discrete system example should also be shown in the tutorial.
• Instead of showing algorithm usage in the Algorithms page, a dedicated Examples page should exist that shows this.
• The example of Davidchack Lai does not seem correct? Or it is confusing. POs with different periods should be colored differently and with a different marker like the standard map example.
• SHouldn't the returned periodic orbits of the Davidchak Lai algorith be filtered to be unique? In the logistic map example it appears we do the filtering after the fact just for plotting.

[JonasKoziorek]

• Floquet multipliers / eigenvalues of the monodromy matrix could be stored in PeriodicOrbit struct once the stability is computed.
• An example of using SchmelcherDiakonos together with PoincareMap should be included in the docs. A test would be good too.
• Extended isdiscretetime shows incorrectly twice in the docs. Docs should show only the docstring of the isdiscretetime(po::PeriodicOrbit).
• Function 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.
• Struct InitialGuess should have a pretty printing enabled so that the digits after decimal point are rounded to (for example) 5.
• Allow passing rootkw keyword argument of PoincareMap to minimal_period.

[JonasKoziorek]

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.

[JonasKoziorek]

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.

[Datseris]

right, all sounds good. If you have time to contribute that would be welcomed of course!

[JonasKoziorek]

Yes, I will look at it in the following days :)

[JonasKoziorek]

SHouldn't the returned periodic orbits of the Davidchak Lai algorith be filtered to be unique? In the logistic map example it appears we do the filtering after the fact just for plotting.

In the logistic map example I filter out POs of a given period. The algorithm returned a vector with POs of various periods. Nevertheless, yes, the DavidchackLai output can be also filtered to be unique.