Suggestions for improving the docs

[nluetts]

Hi :wave:

I am raising this issue as part of the JOSS review https://github.com/openjournals/joss-reviews/issues/6372

I read through the documentation and have a couple of suggestions/questions and found a few of typos. I'll use links to the source files to make locating the respective passages easier:

installation_instructions.md

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/installation_instructions.md?plain=1#L9

• [x] Why clone and not just add CalibrateEmulateSample in package mode (]) to a new Julia project? This works fine with the Sinusoid example. Plus I think it is not desirable to clone the repo into every project that uses CES.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/installation_instructions.md?plain=1#L12

• [x] Better use https for cloning here, since cloning with SSH may fail for users who did not set-up SSH. In contributing.md you use cloning with https. Here (where you want to work on your own fork) cloning via SSH would be more appropriate.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/installation_instructions.md?plain=1#L36-L41

• [x] I think building PyCall is redundant, it is already done by Pkg.instantiate().

examples/sinusoid_example.md

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/examples/sinusoid_example.md?plain=1#L306

• [x] Should this not say "in the true model" instead of "in the Gaussian process"?

Regarding the figures in this section ("Emulator Validation"):

• [x] For comparison it would be better if the color scale would cover the same range from figure to figure, especially when plotting the errors.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/examples/sinusoid_example.md?plain=1#L437-L439

• [x] Maybe add a table with the numeric results, like in the Gaussian process section. This would help to compare the results from Gaussian process and random features.

calibrate.md

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/calibrate.md?plain=1#L14-L16

• [x] A selling point of the package seems to be that the expensive computer model not necessarily needs to be written in Julia. The section "Typical construction of the EnsembleKalmanProcess" just links to

https://clima.github.io/EnsembleKalmanProcesses.jl/dev/ensemble_kalman_inversion/

• [x] which is not saying anything about external models (at least I could not find it).

https://clima.github.io/EnsembleKalmanProcesses.jl/dev/ensemble_kalman_inversion/#Updating-the-Ensemble

• [x] talks about the "dynamic model", but still does not mention the dynamic model being anything else than a Julia function. It would be good to explicitly state that an external model would need to be wrapped into a Julia function and the wrapper would be put in at this stage—if I understand correctly. If not, do you elaborate somewhere how to use an external model?

emulate.md

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/emulate.md?plain=1#L32

• [x] states that all Emulators must use optimize_hyperparameters!(emulator) while the Lorenz example

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/examples/lorenz_example.md?plain=1#L225-L228

• [x] states that "sometimes one must call an optimize step afterwards". Which is it?

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/emulate.md?plain=1#L58-L59

• [x] The importance of providing the covariance matrix is a bit buried here (not mentioned in the examples, I think). Perhaps display a warning (@warn) if no covariance matrix is provided when constructing the Emulator?

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/emulate.md?plain=1#L75-L80

• [x] You should provide type information here, including the return types, so it is easier for the user to implement your interface.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/emulate.md?plain=1#L81

• [x] "Add a new tool as follows:" should better read "Contribute a new tool as follows:" since this seems to be about modifying CalibrateEmulateSample.jl and not the users code. If this is correct, you could also place a link in https://clima.github.io/CalibrateEmulateSample.jl/dev/contributing/#What-to-contribute saying that users can contribute new emulators.

GaussianProcessEmulator.md

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/GaussianProcessEmulator.md?plain=1#L17-L19

• [x] Are there tradeoffs to consider when choosing one implementation over the other?

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/GaussianProcessEmulator.md?plain=1#L108-L109

• [x] Is one not supposed to already know the observational noise? After all, in emulate.md you write:

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/emulate.md?plain=1#L58-L59

• [x] Or is this some other form of noise that is meant here?

random_feature_emulator.md

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/random_feature_emulator.md?plain=1#L73

• [x] The variable name 1d_cov_structure is invalid, must not start with a digit.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/random_feature_emulator.md?plain=1#L106

• [x] csm default = 10.0 but recommended value >0.2? I guess csm is not directly the shrinkage amount, but this remains unclear to me.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/random_feature_emulator.md?plain=1#L118

• [x] Please put a direct link to the relevant EnsembleKalmanProcess documentation, not just the repo. It is otherwise not clear what other keywords are available (and what types are required, e.g. for localization or scheduler).

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/random_feature_emulator.md?plain=1#L179-L183

• [x] This is confusing to me. This example is the "most general" but "far too general". If this should tell the user what not to do, I think it requires one or two sentences of further explanation.

sample.md

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/sample.md?plain=1#L15-L18

• [x] Add using CalibrateEmulateSample.MarkovChainMonteCarlo here, otherwise the user does not know from where to import RWMHSampling or pCNMHSampling. (I needed to look at the markdown file docs/sample.md to find out.)

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/sample.md?plain=1#L85

• [x] The language here (classes, inheritance) is not what you would typically use in the context of Julia, I think. A struct is only a (container) type with no methods directly attached to it (like in a Python class, where the wording would be perfectly fitting). So instead of "RWMetropolisHastings and pCNMetropolisHastings, both of which inherit from the AdvancedMH.MHSampler base class" it would be "RWMetropolisHastings and pCNMetropolisHastings, both of which are subtypes of the AdvancedMH.MHSampler abstract type." But no need to argue about language, feel free to ignore this.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/sample.md?plain=1#L124

• [x] Better link to the docs:

https://turinglang.org/AbstractMCMC.jl/dev/api/#Sampling-a-single-chain

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/sample.md?plain=1#L135

• [x] What exactly do you mean here? Can you link to the functions you are referring to?

API Reference

• [x] The function Emulator (constructor of type Emulator) does not have an API reference, only the type has an API reference, which is a little unfortunate because one can miss the docs in https://clima.github.io/CalibrateEmulateSample.jl/dev/emulate/#Diagonalization-and-output-dimension-reduction when going directly to the API reference.

• [x] The GaussianProcess constructor function also does not have an API reference, although it has a docstring (which the constructor for Emulator is missing).

Typos

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/examples/sinusoid_example.md?plain=1#L23

• [x] "The goal is to estimate the not just point estimates of the parameters"

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/examples/sinusoid_example.md?plain=1#L262

• [x] "with improved scaling properties, making them more suitable for for higher dimensional problems."

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/examples/sinusoid_example.md?plain=1#L354

• [x] Should this be $\delta$ (dollars instead of backticks)?

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/examples/lorenz_example.md?plain=1#L143

• [x] Missing backtick for emulate_sample.jl

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/emulate.md?plain=1#L69

• [x] "To help with poor conditioning of the covariance matrix, users can also standardize each output dimension with by a multiplicative factor given by the elements of factor_vector." (period missing)

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/random_feature_emulator.md?plain=1#L97

• [x] Replace comma with period.

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/sample.md?plain=1#L7

• [x] "acheived"

https://github.com/CliMA/CalibrateEmulateSample.jl/blob/87c069ce531062151a3aa0a5fbc823fbb10292c1/docs/src/sample.md?plain=1#L30

• [x] Capital S in "Sampling method". Above also capital C in "Calibration stage" and capital E in "Emulate stage", but perhaps this is intentional.

[odunbar]

@nluetts Thank you for your detailed combing, I apologize that you encountered so many typos - these are most certainly on me.

If you don't mind, I have added a check-list to your comments to help me mark them off more easily! I'll open a PR for this to address these soon.

[odunbar]

I hope that PR #296 was able to address most of the issues raised. You may consult the PR comment, code diffs, or the docs linked in the PR comment for quick validation of what I have done Thanks again for the time you placed in going through the many pages. I leave here one or two clarifying comments.

• Regarding trade-offs of Emulators: I have only added a note saying that users should give both a go to see what is effective. I don't want to explicitly talk about what is better as these are commonly problem specfic, furthermore the packages do evolve over time and change in performance in different cases.
• Regarding csm, it is a number of samples needed to approximate a covariance matrix. The 0.2 is a measure between 0 and 1 of the amount of shrinkage. With huge amounts of samples the shrinkage = 0, with very few samples shrinkage = 1. This is now explained in more detail

The following are not yet resolved issues - as they involve modifying code that is not just in docs/ and so will be completed in later PRs

• Requires change/rerun of plots for sinusoid_example For comparison it would be better if the color scale would cover the same range from figure to figure, especially when plotting the errors.
• Requires change/rerun of plots for sinusoid_example Maybe add a table with the numeric results, like in the Gaussian process section. This would help to compare the results from Gaussian process and random features.
• Requires change in Emulator.jl The importance of providing the covariance matrix is a bit buried here (not mentioned in the examples, I think). Perhaps display a warning (@warn) if no covariance matrix is provided when constructing the Emulator?
• Requires change in Emulator.jl The function Emulator (constructor of type Emulator) does not have an API reference, only the type has an API reference, which is a little unfortunate because one can miss the docs in https://clima.github.io/CalibrateEmulateSample.jl/dev/emulate/#Diagonalization-and-output-dimension-reduction when going directly to the API reference.

[odunbar]

I believe we have addressed everything, Please close the issue if you are satisfied.

[nluetts]

Thanks for clarifying, @odunbar , this looks all good to me, so I close this issue.