Skip to content
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

Open
10 of 46 tasks
brosaplanella opened this issue Oct 2, 2023 · 5 comments
Open
10 of 46 tasks

Improve documentation (ongoing) #3392

brosaplanella opened this issue Oct 2, 2023 · 5 comments
Assignees
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours ongoing priority: medium To be resolved if time allows

Comments

@brosaplanella
Copy link
Member

brosaplanella commented Oct 2, 2023

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.

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

  • The overall structure of the classes is not documented or explained. Needs to be a diagram, plus a lot of explanation on the class structure and dependencies (@martinjrobins )
  • we need to make clear which parts of the API docs are relevent to users, perhaps a list of user-facing classes in the API docs main page. (@ejfdickinson, edit by @martinjrobins ) #4535
  • Generally make clear where classes implement getitem and/or setitem and so these are the expected calling patterns. (from @ejfdickinson )
  • 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 and auxillary_domains are confusing and we should resolve this asap (i.e. deprecate and remove domains and auxillary_domains (@martinjrobins)
  • pybamm.Variable. domain arg needs to be documented (or removed entirely in favor of domains) (@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 in pybamm.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

  • make explicit that pybamm.Solution implements a dict of pybamm.ProcessedVariable instances, so that the standard calling pattern is using getitem() on the pybamm.Solution and then using a method of the pybamm.ProcessedVariable class. (from @ejfdickinson )
  • pybamm.ProcessedVariable documents the public attribute data as "the same as entries" but doesn't document entries. (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 and pybamm.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 and pybamm.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 be str rather than string. Also this needs to list possible string values this can take
  • pybamm.Experiment.read_termination, arg termination not documented or given a type. The return value is also not documented #3724
  • pybamm.BaseSolver.solve. Arg t_eval can be None, a list of numeric values or a numpy ndarray. Arg calc_sensitivities is optional
  • pybamm.BaseSolver.step. Arg `save is optional
  • pybamm.ScipySolver. Link to scipy.integrate.solve_ivp and format as code
  • pybamm.JaxSolver. Arg method is optional. Link to jax.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 in jax_bdf_integrate.py
  • pybamm.IDAKLUSolver. Arg root_method: should link to algebraic solver class and scipy.optimize.root (format the latter as code).
  • pybamm.IDAKLUSolver. Arg extrap_tol defaults to None, not zero. Need to explain what None means, and what extrapolation is
  • pybamm.QuickPlot. Arg labels should be same length as solutions, raises a ValueError if not. There are various bits of code in this docstring, which should be formated as inline code
  • pybamm.QuickPlot.create_gif. remove round brackets around optional keyword ([Bug]: Fix the Bug in pybamm.QuickPlot.py file #3754)
  • pybamm.QuickPlot.dynamic_plot. Args are optional ([Bug]: Fix the Bug in pybamm.QuickPlot.py file #3754)
  • pybamm.QuickPlot.get_spatial_var. Should be private (name start with underscore) ([Bug]: Fix the Bug in pybamm.QuickPlot.py file #3754)
  • pybamm.QuickPlot.plot. Document dynamic arg ([Bug]: Fix the Bug in pybamm.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 to matplotlib.pyplot.plot, not plt.plot. See also pybamm.plot2D
  • pybamm.plot_voltage_components. Arg kwargs_fill, format ax.fill_between as code and link to matplotlib docs for this function.
  • pybamm.plot_summary_variables. Arg kwargs_fig, format plt.subplots as code and link to matplotlib docs. I also think the actual name of this function shouldn't start with plt (presumably matplotlib.pyplot.subplots?)
  • pybamm.get_git_commit_info. document return type
  • pybamm.rmse. document return type and args types
  • pybamm.Timer. example is not properly formatted. In Timer.time, meth:reset() is not properly formated
  • pybamm.FuzzyDict.copy. needs docstring
  • pybamm.FuzzyDict. get_best_matches. document key arg
  • pybamm.FuzzyDict. search. document args. format code properly
  • pybamm.load. Needs documentation, unclear that it is just a thin wrapper around pickle.load, and filename arg is not documented
  • pybamm.install_jax. Document arguments arg, unclear on the type.
  • pybamm.have_jax and pybamm.is_jax_compatible, document return type (bool?)
  • pybamm.steps mention that drive cycles should start at t=0 (see More informative error extrapolation drive cycle #3612).
@brosaplanella brosaplanella added difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows hacktoberfest labels Oct 2, 2023
@brosaplanella brosaplanella changed the title Improve documentation Improve documentation (ongoing) Oct 2, 2023
@brosaplanella brosaplanella pinned this issue Oct 2, 2023
@ejfdickinson
Copy link

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 pybamm.step._Step() that is not supposed to be called in user code. Just list the permitted value formats and kwargs that can be used in the pybamm.step.[...] options.

@ejfdickinson
Copy link

Generally make clear where classes implement __getitem__ and/or __setitem__ and so these are the expected calling patterns.

Especially, make explicit that pybamm.Solution implements a dict of pybamm.ProcessedVariable instances, so that the standard calling pattern is using __getitem__() on the pybamm.Solution and then using a method of the pybamm.ProcessedVariable class.

@ejfdickinson
Copy link

pybamm.ProcessedVariable documents the public attribute data as "the same as entries" but doesn't document entries.

@valentinsulzer
Copy link
Member

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.
@ejfdickinson seems like you have lots of good suggestions, would you be available to mentor?

@ejfdickinson
Copy link

ejfdickinson commented Oct 5, 2023

@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.

My recommendation for the best thing you can do is get a couple of students who are good at Python but have never used PyBaMM before, and have them reproduce a few modelling papers, especially those that don't use PyBaMM. Then just sit over their shoulders and work out what is clear and what is not.

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.

@kratman kratman unpinned this issue Jan 3, 2024
@kratman kratman pinned this issue Jan 3, 2024
@Jiabin-Wang Jiabin-Wang mentioned this issue Apr 5, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours ongoing priority: medium To be resolved if time allows
Projects
Status: In Progress
Development

No branches or pull requests

5 participants