Documentation with minimal example

[JanJereczek]

The built documentation is here: https://juliadynamics.github.io/TransitionIndicators.jl/previews/PR19/

---

Major changes:

• logo (animated and static)
• overview documented in introduction of index.md
• minimal example in documentation
• docstrings expanded
• load_data.jl has function to load time series of minimal example. Maybe more time series will be included in future.

Minor changes:

• minor renaming of some functions.
• simplified use of recomputed_ridge_slope()`
• functions for statistical moments of higher order and correlation. I think we should actually reexport these from StatsBase.jl.

[Datseris]

can you please resolve the conflicts @JanJereczek ? (And please always ensure this is the case when you make a PR, otherwise CI doesn't run)

[JanJereczek]

Should be ok now! I will make sure there is no conflict in the future

[Datseris]

The built documentation is here: https://juliadynamics.github.io/TransitionIndicators.jl/previews/PR19/

which I'll read and give feedback about soon! I promise! Hopefully before monday!

[Datseris]

@JanJereczek thanks a lot for putting the effort into the docs. It looks good, but it also can get better.

Alright, here we go for the review:

• The introduction has too much text about science. We can cut this to one or two sentences.
• We shouldn't burden the repo with large files at the moment (figures). First, move the logo to the JuliaDynamics repository that holds other resources like timeseries, and use that. Second I'd say that for now we can remove the logo icon (that is shown on the left sidebar) because it isn't final yet, but it will forever remain in the git history if we commit it.
• "Eases the reproducibility thanks to a clear syntax and a simple installation.". I would also add "and RNG-seeded surrogate generation".
• Similarly, we shouldn't commit the flow chart into the repo. if you really want it you can also put it in the JuliaDynamics repo and link it here.
• The textbox titled "adaptability" does not seem to be useful at the moment. You can make this a single sentence in the above enumerated list, saying something "the fowchart is flexible; boxes can be modified or skipped alltogether".
• The example needs some work. There is too much code without explanmation at the moment. You should separate the example into individual example blocks where you explain each step, step by step.
• Function name init_metaanalysis_params is inappropriate. If we have a type MetaanalysisParameters then this function is its constructor and should have the same name. Morever, the names should be specific. This meta analysis is specifically for significance. So I would say, just make a struct SignificanceMetaParams and use this name for the function as well.
• I don't understand what's the purpose of precomputed_ridge_slope and why we can't use ridge_slope directly. In general keep this in mind: the less amount of names we have in the API, the better.
• You say "For the surrogate generation, you can use any technique defined in TimeseriesSurrogates.jl.". Here is an important lesson: words that are concrete and specific in code are always much more powerful and clear than words that are written in english. Instead of saying "any technique" say "any subtype of Surrogate". This is very specific, because Surrogate is something completely specific: sometjhing defined concretely within the package TimeseriesSurrogates.jl
• confidence_intervall has two l
• This docstring is really hard to understand: "measure_significance(res, significance_metric), Compute the significance_metric on the results res of an indicator metaanalysis". What is an indicator metaanalysis? Be specific. Is it the output a specific function? If yes, explicitly say so and link this function with @ref. Is it a specific type? if so, explicitly say so. In general: be explicit: code is better than english.
• Does the function measure_signficance has a reason to exist?
• The function measure_significances has no reason to exist though, right? isn't it the broadcasting of the measure_significance?

the most important part I think is to re-write the tutorial so that it holds the hand of the user step by step all the way through.

[Datseris]

after the meeting: name SignificanceHyperParams is what we thought was best

[JanJereczek]

All comments have been embedded! Ready for review :)

This versions also includes the struct/function dispatch for RidgeRegression. I created FunctionalStruct <: Function as abstract type so that f(x::Vector, metric::Function)accepts such functions. Not sure it is the way to go though...

[Datseris]

The solution is to define

struct YourName <: Function
# .,,.,.,
end

you don't need the extra type hierarchy.

[JanJereczek]

Hahaha, I went rather for: why make things simple when you can make them complicated?

Thanks for the comment, it is now implemented without the introduction of a new type!

[Datseris]

@JanJereczek very important, read this: https://docs.julialang.org/en/v1/manual/style-guide/#Write-functions-with-argument-ordering-similar-to-Julia-Base

practically every function that accepts a function as an argument, this argument should be first, not second. Like in our ...(x, indicator, ...).

[Datseris]

@JanJereczek another very important thing. Stop declaring types. You only should declare types in case of new method definition or in case of mandatory subtyping. This means, you should be as general with your type delcerations as possible. If you can avoid declering a type in a function definition, then you should. If you can use a supertype, you should. Always prefer AbstractVector instead of Vector unless there is an absolutely specific reason to use Vector.

[Datseris]

(I am now workign intensively o nthe PR, please don't push anything)

[Datseris]

Another comment: the function is_equispaced violates the "do 1 thing rule". Its name also suggests that it should return a boolean only.

[Datseris]

Another comment: the way you have defined SignificanceHyperParams is problematic. The significance is tested with the surrogates and a significance test. The SignificanceHyperParams should have nothing to do with the creation of the indicator timeseries. This should be a completely independent step. The significance struct should have the wv_evolution_width stuff.

By the way, the names at the moment are incredibly hard to parse. What is "evolution_width" referring to...? It is very hard to understand that this is about the length of the "change" estimating function, that estimates how much an indicator changes.

[Datseris]

Alright, I will establish the following names:

• input timeseries (self explanatory)
• indicator timeseries: the result of applying an indicator over a sliding window of the input timeseries
• change metric timeseries: the result of applying a change metric (such as slope or max absolute difference) over a sliding window of the indicator timeseries.

The change metric timeseries is used to detect significant transitions.

[JanJereczek]

Hi George,

Thanks for all the detailed corrections and explanations! I'll let you finish what you are doing. When you're done let me know and I would gladly correct anything that still should be.

For the future: I'll read more code from JuliaDynamics packages to internalize better the best practices and hopefully reduce the time you spend on corrections!

[Datseris]

Another comment: don't leave TODO comments like "implem,ent lfps". Make them GitHub issues instead.