-
-
Notifications
You must be signed in to change notification settings - Fork 548
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
Improve documentation (ongoing) #3392
Comments
I'd widen the one relating to "experiment steps" to make clear that it feels like bad practice to give detailed documentation for a constructor |
Generally make clear where classes implement Especially, make explicit that |
|
We could look into Google Season of Docs for this https://developers.google.com/season-of-docs/docs/timeline Seems like there is a lot to be done to follow best practices, not requiring domain knowledge. |
@tinosulzer Yes in principle, but can you suggest what the time commitment and overall working period would be? My suggestions are all provoked empirically by using the library for different tasks! I made a suggestion in DM to @brosaplanella which I'll copy here on how to identify areas where documentation is misaligned with expected use patterns.
Here "sit over their shoulders" is not to be understood literally! Just get people writing down where things are non-obvious, touching base with power users, and then working out why the "power use" implementation couldn't be reasoned from the docs. |
There are quite a few features that are not in the docs, and which would be fairly minor fixes. Opening a list here so we can keep track of them as we go and fix them in batches. Feel free to add more in the comments, and we can update the list.
print_parameter_info
not documented inbase_model.py
(Issue 3361 - Improve print_parameter_info functionality #3584)User Guide
Submodel docs: At the moment the documentation for the base models in pyamm just points to the relevent papers, but this is not really sufficient given that users can customise each model via submodels. As a proposal, I think there should be hierarchical user documentation on each of the models. At the top level there are the three main default models (SPM, SPMe, DFN), with a description of the relevent equations that is divided up using the submodel structure, so that a user can see the list of equations that are in each submodel, along with the parameters in that submodel (at the moment, its very diffiicult to know what parameters are needed for what submodels). Below this there should be an individual discussion of each submodel and the different options for each one. For each option the corresponding equations and parameters should be provided. Incompatible submodels should be highlighted. (edit by @martinjrobins)
Getting started tutorials: The getting started tutorials should have an expanded section on submodels, how to vary them, and how to determine what parameters are needed by your particular model configuration. (@martinjrobins)
Getting started tutorials: These docs introduce a lot of user-facing pybamm classes like
Simulation
, and should link to the API docs for these classes so that users can see the full range of options for these classes/functions. (edit by @martinjrobins)Getting started tutorials: The "setting parameters" section needs to describe input parameters (@martinjrobins) #4532
The user-guide, or main page, needs a "How to get help" section explictly pointing to discussions, issues, slack channel and explaining how to use each (@martinjrobins)
In the install from source we should make clearer that you do not need to create a virtual environment if using nox, as nox will create one for you.
API Docs
pybamm.Symbol
. This is only a one-liner docstring, needs to explain the overall expression tree structure, with links to example notebooks, and include some examples of use, particularly explaining how to use all the overloaded operators (@martinjrobins )pybamm.Symbol
and all inherited classes:domain
,domains
andauxillary_domains
are confusing and we should resolve this asap (i.e. deprecate and removedomains
andauxillary_domains
(@martinjrobins)pybamm.Variable
.domain
arg needs to be documented (or removed entirely in favor ofdomains
) (@martinjrobins)pybamm.BaseBatteryModel
. Only a one-liner docstring, needs significant expansion. Explain the structure and purpose of the class, and examples of use. Link to example notebooks for more examples (@martinjrobins)pybamm.BaseBatteryModel
. The info here (mainly inpybamm.BatteryModelOptions
) is essential, user-facing info on the submodel configuration options, so should be much more visible (e.g. much of it should be copied over to a section in the user guide). There is no information on which submodel configurations conflict with each other, and only some of the options indicate what equations are added/removed, or what parameters are added/removed. I feel like each option should be its own page / example notebook, linking to publications where relevent and there should be much more detailed information at the equation/parameter level so users know exactly what model they are constructing (@martinjrobins )API Docs - Small fixes suitable for first time contributors
pybamm.Solution
implements a dict ofpybamm.ProcessedVariable
instances, so that the standard calling pattern is usinggetitem()
on thepybamm.Solution
and then using a method of thepybamm.ProcessedVariable
class. (from @ejfdickinson )pybamm.Simulation.save
,filename
arg is not documented (pybamm.Simulation.save, filename arg is not documented #3602)pybamm.Simulation.set_parameters
,pybamm.Simulation.set_up_and_parameterise_experiment
andpybamm.Simulation.set_up_and_parameterise_model_for_experiment
should be private (i.e. begin with underscore_
) and not documented (tracked by [Bug]:pybamm.Simulation.set_parameters
,pybamm.Simulation.set_up_and_parameterise_experiment
andpybamm.Simulation.set_up_and_parameterise_model_for_experiment
should be private (i.e. begin with underscore _) and not documented #3751)pybamm.Experiment
.period
arg type is not linked, perhaps should bestr
rather thanstring
. Also this needs to list possible string values this can takepybamm.Experiment.read_termination
, argtermination
not documented or given a type. The return value is also not documented #3724pybamm.BaseSolver.solve
. Argt_eval
can be None, a list of numeric values or a numpyndarray
. Argcalc_sensitivities
is optionalpybamm.BaseSolver.step
. Arg `save is optionalpybamm.ScipySolver
. Link toscipy.integrate.solve_ivp
and format as codepybamm.JaxSolver
. Argmethod
is optional. Link tojax.experimental.odeint
and format as code. For method=‘BDF’ option summarise method used (use docstrings in jax_bdf_integrate.py) or simply link to the docstrings injax_bdf_integrate.py
pybamm.IDAKLUSolver
. Argroot_method
: should link to algebraic solver class andscipy.optimize.root
(format the latter as code).pybamm.IDAKLUSolver
. Argextrap_tol
defaults toNone
, not zero. Need to explain whatNone
means, and what extrapolation ispybamm.QuickPlot
. Arglabels
should be same length assolutions
, raises aValueError
if not. There are various bits of code in this docstring, which should be formated as inline codepybamm.QuickPlot.create_gif
. remove round brackets around optional keyword ([Bug]: Fix the Bug inpybamm.QuickPlot.py
file #3754)pybamm.QuickPlot.dynamic_plot
. Args are optional ([Bug]: Fix the Bug inpybamm.QuickPlot.py
file #3754)pybamm.QuickPlot.get_spatial_var
. Should be private (name start with underscore) ([Bug]: Fix the Bug inpybamm.QuickPlot.py
file #3754)pybamm.QuickPlot.plot
. Documentdynamic
arg ([Bug]: Fix the Bug inpybamm.QuickPlot.py
file #3754)pybamm.plot
. Format bits of code as inline code,ax
Arg should link to matplotlib Axis docs,kwarg
arg should refer tomatplotlib.pyplot.plot
, notplt.plot
. See alsopybamm.plot2D
pybamm.plot_voltage_components
. Argkwargs_fill
, formatax.fill_between
as code and link to matplotlib docs for this function.pybamm.plot_summary_variables
. Argkwargs_fig
, formatplt.subplots
as code and link to matplotlib docs. I also think the actual name of this function shouldn't start withplt
(presumablymatplotlib.pyplot.subplots
?)pybamm.get_git_commit_info
. document return typepybamm.rmse
. document return type and args typespybamm.Timer
. example is not properly formatted. InTimer.time
, meth:reset() is not properly formatedpybamm.FuzzyDict.copy
. needs docstringpybamm.FuzzyDict. get_best_matches
. documentkey
argpybamm.FuzzyDict. search
. document args. format code properlypybamm.load
. Needs documentation, unclear that it is just a thin wrapper aroundpickle.load
, andfilename
arg is not documentedpybamm.install_jax
. Documentarguments
arg, unclear on the type.pybamm.have_jax
andpybamm.is_jax_compatible
, document return type (bool
?)pybamm.steps
mention that drive cycles should start att=0
(see More informative error extrapolation drive cycle #3612).The text was updated successfully, but these errors were encountered: