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

Documentation #33

Open
murphyqm opened this issue Jan 29, 2021 · 10 comments
Open

Documentation #33

murphyqm opened this issue Jan 29, 2021 · 10 comments

Comments

@murphyqm
Copy link
Owner

Add full documentation including community guidelines, list here.

@andreww
Copy link
Collaborator

andreww commented Jan 29, 2021

The big bit of work there is the "Functionality documentation", but from what I can see you have most of this in the doc strings (which look good). One way to use these is with pydoc (scripting something to run "pydoc -w $file" on each python file will generate a load of HTML documentation). However, I suspect there are much better ways to do this via http://docs.readthedocs.io

@andreww
Copy link
Collaborator

andreww commented Jan 29, 2021

Also, a very useful form of documentation is an executable notebook. Once the packaging is done this is easy to set up via binder. Basically you can build a URL that embeds the notebook location on github, following that link gives the reader a running notebook to play with.

@murphyqm
Copy link
Owner Author

Have Sphinx up and running, auto-building docs from docstrings. Issues with the mantle_timestepping module as this contains relative imports (it imports core module and mantle properties module); however, on the to-do list is changing the way this function uses the core object (moving instantiation out of the timestepping function and instead feeding in a calleable argument), and will do similar with the mantle_properties object (improves modularity) too, which results in not requiring relative imports. Can look into fixing the relative imports issue more if when all the refactoring and tidying up is finished and it ends up still being a problem.

Have not yet set up a github workflow to rebuild docs when changes are pushed to the repo, and haven't yet linked it to read the docs. Need to rewrite/add detail to some doc strings, and remove comments that are accidentally being pulled into the documentation.

@murphyqm
Copy link
Owner Author

Also sphinx has an extension that builds a html gallery of examples from a defined directory of python scripts; can point this to jupyter nb files so they are included tidily in the documentation (as well as having a live version on binder) - https://sphinx-gallery.github.io/stable/index.html

@andreww
Copy link
Collaborator

andreww commented Apr 19, 2021

Nice! It looks like that's how Matplotlib makes their examples.

@murphyqm murphyqm added this to the JOSS submission milestone Apr 21, 2021
@murphyqm
Copy link
Owner Author

Doc issues for pytesimal package imports

When certain packages (including pytesimal) are imported, sphinx autodoc fails, because these packages haven't been added to it's requirements.txt.

Commenting out from . import pytesimal.blah from the and then building html docs works to create a correct local html version of the docs, however read the docs builds from the source script in the repository, so this fix doesn't work for the hosted version of the docs.

Going to use their mock functionality to fake imports of certain dependencies (more info on their FAQ page). Not sure if this will work with the pytesimal package... but hopefully. There's probably a way to define the package name somewhere in the sphinx config files, so it just accepts a from . import statement.

@andreww
Copy link
Collaborator

andreww commented Apr 29, 2021

I'm putting any minor documentation issues on this branch: https://github.com/murphyqm/pytesimal/tree/amw-docs - I'll generate a pull request once I think I've found them all.

@andreww
Copy link
Collaborator

andreww commented May 28, 2021

Probably need to go through all the docstrings to check:

  • Do we specify units wherever it's sensible to do so
  • Do we specify call sigs when we pass in functions or objects

We probably also need to think about how to explain the coupling in a useful way ... this will be needed in the paper and (possibly) in an example.

@murphyqm
Copy link
Owner Author

Just combing through docstrings now, adding units. Does the below sound sensible for specifying call signatures for the material properties in the discretisation function, in the numerical_methods module? I essentially modeled it off the descriptions you wrote for top_mantle_bc and bottom_mantle_bc in the same function, but wasn't sure if I've formatted the call sigs correctly (since it's referencing a method of an object as opposed to a standalone function).

cond : callable
        Callable function or method that defines the mantle conductivity. The
        calling signature is cond.getk(temperatures[radial_index,
        timestep_index]), where temperatures is the temperatures array,
        radial_index is the row index of the radius, and timestep_index is the
        column index of the timestep, that define the value in temperatures at
        which conductivity should be evaluated. The function must return a
        value for conductivity in in W m^-1 K^-1.
heatcap : callable
        Callable function or method that defines the mantle heat capacity. The
        calling signature is heatcap.getcp(temperatures[radial_index,
        timestep_index]), where temperatures is the temperatures array,
        radial_index is the row index of the radius, and timestep_index is the
        column index of the timestep, that define the value in temperatures at
        which heat capacity should be evaluated. The function must return a
        value for heat capacity in J kg^-1 K^-1.
dens : callable
        Callable function or method that defines the mantle density. The
        calling signature is dens.getrho(temperatures[radial_index,
        timestep_index]), where temperatures is the temperatures array,
        radial_index is the row index of the radius, and timestep_index is the
        column index of the timestep, that define the value in temperatures at
        which heat capacity should be evaluated. The function must return a
        value for density in kg m^-3.

@andreww
Copy link
Collaborator

andreww commented Jul 19, 2021

Do you need to pass in objects here, or could you pass in the method itself as a callable? Looking at cond for example,
do you ever use anything else from the object or do you only use the getk method?

If you need the whole object inside discretisation I would change cond : callable to cond : conductivity object and say that the argument has to be an instantiated object from the class (linking to the class documentation if possible). However, I think you give users more freedom if you only need a callable. In which case I would change the code to accept a callable (so you put cond.getk in the real argument list, without the brackets) and documentation just specifies that the callable must return the conductivity given a scalar temperature. You would then probably want to add a comment to the effect that it is convenient for this passed function to be the getk method from a conductivity object so that the user can easily set things up etc. I guess as you have cond.getkdT you'll either need the whole object, or will need to pass in two functions. I would probably follow the various numpy optimiser API and pass in two functions (you could always make the derivative optional and if it's not provided find the derivative numerically).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants