Alternative automatic API ref doc generation #1485
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1485 +/- ##
===========================================
+ Coverage 95.26% 95.92% +0.66%
===========================================
Files 82 86 +4
Lines 3736 3854 +118
===========================================
+ Hits 3559 3697 +138
+ Misses 177 157 -20
Continue to review full report at Codecov.
|
Fixes a syntax error in the Makefile introduced by GPflow#1471. Also adds a `format-check` target for black --check and a `check-all` target that runs format-check, type-check and the pytest tests.
* bring contributing.md up to date (resolves GPflow#1474) Co-authored-by: joelberkeley-pio <joel.berkeley@prowler.io>
|
Overall this looks very good - I've noticed that it doesn't show the type annotations in the signature anymore, but we can fix that by moving to proper Sphinx docstrings and explicitly having the More problematic is that as you point out it does not play nicely with the multiple dispatch. Would it be possible to make use of your approach for most of the modules, and have our custom multipledispatch-handling script for where it's needed? |
GPR used the X.shape attribute instead of calling tf.shape(X); this prevented using it with Variable data, which is now successfully demonstrated in a test.
Doc improvement: Fixes some typos in the Scipy docstring and adds a note on scipy minimize()'s callback argument vs GPflow Scipy().minimize()'s step_callback. Cleans up types in the module. Backwards-incompatible change: the `step_callback` that can be passes to the Scipy.minimize() method is changed to take its arguments by position, not name; in line with the StepCallback type signature (and in line with Python's typing.Callable documentation which suggests that callbacks should take arguments preferentially by position, not name).
…Pflow#1489) As discussed in GPflow#878, GPflow's NaturalGradient optimizer does not implement the diagonal covariance parametrization (`q_diag=True`). This PR clarifies this in the documentation and adds extra shape checks.
Note that this links to a *different* article from the previous one - we will add the old link back in as well once it is back online.
Release 2.0.5
Doc update. Resolves GPflow/docs#4 * updated models.gpr docstring * Elaboration. Co-authored-by: Pola Elisabeth Schwobel <posc@MacBook-Pro.local> Co-authored-by: Mark van der Wilk <markvanderw@gmail.com>
Clarify template to make it easier for contributors to fill in relevant information.
Update develop with master
There was a problem hiding this comment.
I largely echo what Ti said above.
The documentation certainly looks better, and builds easier but it doesnt not having the multiple dispatch is a problem. I think having this + generate_rst_py is very confusing and I very much think we should only choose one.
Personally, I'm not convinced that the individual multiple dispatch functions are really considered public API, since many have leading underscore and the whole point of multiple dispatch is to have one API to get one thing done, even if the implementations differ.
I'd be temped to not document these methods thoroughly and maybe just have a clearer docstring saying that these are dispatched functions and what they do?
| always_document_param_types = True | ||
| # autoclass_content = 'both' | ||
| # Mappings for sphinx.ext.intersphinx. Projects have to have Sphinx-generated doc (.inv file) | ||
| intersphinx_mapping = { |
There was a problem hiding this comment.
what exactly is this doing? do we need an inv file?
There was a problem hiding this comment.
Intersphinx seems quite useful to me - it means that when you reference a third party object from Python or Numpy then the documentation automatically links through to the Python/Numpy documentation for that object. It's a nice to have, I guess, rather than essential - but it makes for a nice user experience.
Unfortunately, the .inv file for Tensorflow, the library that gets referenced a lot, is non-functional at the moment, and the original developer is unresponsive. It may be possible to build and host the .inv file for TF ourselves, or we could just remove the TF mapping.
There was a problem hiding this comment.
Personally, I'm not convinced that the individual multiple dispatch functions are really considered public API, since many have leading underscore and the whole point of multiple dispatch is to have one API to get one thing done, even if the implementations differ.
I would agree with this. My original plan was to write some custom words in the __init__.py file for each of the modules that uses multiple dispatch summarising what they were actually doing and include it in this PR, but I ran out of time.
It seems a shame to write custom summaries to document an API when a combination of Sphinx and the docstrings should be doing it automatically for us, but I would argue that even now, the current documentation doesn't do a great job of explaining what's going on in the dispatch modules. There's a lot of noise and it's hard to consume.
| @@ -50,6 +50,8 @@ clean: | |||
|
|
|||
| .PHONY: html | |||
| html: | |||
| rm -rf build | |||
There was a problem hiding this comment.
do we really need this here? how was it working before?
There was a problem hiding this comment.
Don't need this, no... it was just a safety check for me when building locally that each new build was completely splatting the old... I don't think it does any harm though.
| @@ -18,7 +18,7 @@ To compile the GPflow documentation locally: | |||
|
|
|||
| 2. Install doc dependencies | |||
| ``` | |||
| pip install sphinx sphinx_rtd_theme numpydoc nbsphinx sphinx_autodoc_typehints ipython jupytext jupyter_client ipywidgets | |||
| pip install sphinx sphinx_rtd_theme numpydoc nbsphinx sphinx-autodoc-typehints sphinx-autopackagesummary ipython jupytext jupyter_client ipywidgets matplotlib sklearn tensorflow_datasets | |||
There was a problem hiding this comment.
do we also need to install all these doc dependencies (matplotlib, tensorflow_datasets)? isn't there a docs_require.txt or smth we can put here instead? What does RTD use?
There was a problem hiding this comment.
Building the doc locally (even now, irrespective of this PR) fails unless matplotlib, sklearn and tensorflow_datasets are installed - I guess these are required by the Notebooks in the doc/source/notebooks directory. So I added them here as it was pretty frustrating.
…low#1522) * pin cloudpickle==1.3.0 as temporary workaround for tensorflow/probability#991 to unblock our build (to be reverted once fixed upstream)
Adds Copyright notice and Apache license text (short form) to the preamble of all python files underneath gpflow/. Also unifies the copyright notice to simply state "The GPflow Contributors" to simplify maintenance of the copyright notices (this does not affect who holds copyright).
Merge master back into develop
* refactoring scalar likelihood * adding dtype casts to quadrature * black Co-authored-by: Gustavo Carvalho <gustavo.carvalho@delfosim.com> Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com>
* HeteroskedasticLikelihood base class draft * fixup * cleanup * cleanup heteroskedastic * multioutput likelihood WIP * Notebook exemplifying HeteroskedasticTFPDistribution usage (GPflow#1462) * fixes * typo fix; reshaping fix * notebook showing how to use HeteroskedasticTFPDistribution likelihood * converting to .pct.py format * removed .ipynb * better descriptions * black auto-formatting Co-authored-by: Gustavo Carvalho <gustavo.carvalho@delfosim.com> * note and bugfix * add comment * Adding heteroskedastic tests (GPflow#1508) These tests ensure that heteroskedastic likelihood with a constant variance, will give the same results as a Gaussian likelihood with the same variance. * testing * added QuadratureLikelihood to base, refactored ScalarLikelihood to use it * fix * using the first dimension to hold the quadrature summation * adapting ndiagquad wrapper * merged with gustavocmv/quadrature-change-shape * removed unecessary tf.init_scope * removed print and tf.print * removed print and tf.print * Type annotations Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * Work * Fix test * Remove multioutput from PR * Fix notebook * Add student t test * More tests * Copyright * Removed NDiagGHQuadratureLikelihood class in favor of non-abstract QuadratureLikelihood * _set_latent_and_observation_dimension_eagerly * n_gh ---> num_gauss_hermite_points * removed NDiagGHQuadratureLikelihood from test * black * bugfix * removing NDiagGHQuadratureLikelihood from test * fixed bad commenting * black * refactoring scalar likelihood * adding dtype casts to quadrature * black * small merging fixes * DONE: swap n_gh for num_gauss_hermite_points * black Co-authored-by: ST John <st@prowler.io> Co-authored-by: gustavocmv <47801305+gustavocmv@users.noreply.github.com> Co-authored-by: Gustavo Carvalho <gustavo.carvalho@delfosim.com> Co-authored-by: st-- <st--@users.noreply.github.com> Co-authored-by: joshuacoales-pio <47976939+joshuacoales-pio@users.noreply.github.com>
Follow-up to GPflow#1553
* Global constant for num_gauss_hermite_points default * QuadratureLikelihood with dependency injection (in preparation for unifying with MonteCarloLikelihood) * ScalarLikelihood as QuadratureLikelihood subclass (with _quadrature_dim/_quadrature_log_prob/_quadrature_reduction) * HeteroskedasticTFPConditional: rename argument to `scale_transform` and remove undocumented scaling * DeprecationWarning for deprecated code
* GPflow now includes an isort check in its CI build. Run with `make format` / `make format-check`.
…ll_(output_)cov=True (GPflow#1582) Closes GPflow#1569 Co-authored-by: ST John <st@secondmind.ai>
* automatically add bugs to bug project board * disable blank issues Co-authored-by: ST John <st@secondmind.ai>
* fix heading levels * add cross-references
* move Version check inside reset_cache_bijectors to not confuse Sphinx * revert non-working "projects: 7" line in bugs issue template * fix code-block type in intro.rst
…ng (GPflow#1586) Co-authored-by: ST John <st@secondmind.ai> Co-authored-by: Sandeep Tailor <s.tailor@insysion.net>
…low versions (GPflow#1585) We used to tile in gauss_kl() to work around TensorFlow's lack of broadcasting. Since TensorFlow 2.2, this is finally working, and this PR removes the tiling for TensorFlow>=2.2. Resolves GPflow#1321. Co-authored-by: ST John <st@secondmind.ai>
Co-authored-by: ST John <st@secondmind.ai>
Resolves GPflow#818 Co-authored-by: ST John <st@secondmind.ai>
* update doc requirements * update copyright
…operty (GPflow#1594) Adds support for inducing variables with dynamically changing shape. Change usage from `len(inducing_variable)` to `inducing_variable.num_inducing` instead. Resolves GPflow#1578.
…com:JamesALeedham/GPflow into jamesleedham/alternative-api-doc-generation
Automatically generating API reference doc rather than using
generate_module_rst.pyfileDescription
Enhancement. There are pros and cos to this new method. See https://docs.google.com/document/d/1AIV8-nGOAzqzrzcA8mxpcgdGKDKyQ9wpjVweCyFbl1g/edit for a comparison with the current method.
Minimal working example
To build the docs locally, don't run
generate_module_rst.py. Instead, follow these instructions: https://github.com/JamesALeedham/GPflow/blob/33a89cb9b4ea23d57160492f27836d29691a4178/doc/README.md