overhaul gp docstrings #6609

daniel-saunders-phil · 2023-03-17T20:25:29Z

What is this PR about?

#6608 noted some motivation for updating the docs throughout the GP module. Much of the work is just style changes. However a couple of new questions came up as I worked through. I'm marking this as a draft pull request until I can get some input:

is_observed= argument is used on MarginalKron.marginal_likelihood and also Marginal.marginal_likelihood. In both cases it is deprecated. But on MarginalKron, there is still a docstring for the argument and on Marginal there isn't. Surely we should standardize but which direction is preferred? I imagine we should eliminate the doc on MarginalKron.
The marginal GPs both have a predict method that can take a model argument. But no description is provided. I imagine the right description is something like the way we talk about it around the samplers - model : Model, optional if in with context

codecov · 2023-03-17T20:38:49Z

Codecov Report

Merging #6609 (8cdc77c) into main (c7279b5) will decrease coverage by 6.31%.
The diff coverage is 77.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6609      +/-   ##
==========================================
- Coverage   87.09%   80.78%   -6.31%     
==========================================
  Files          93       94       +1     
  Lines       15737    15927     +190     
==========================================
- Hits        13706    12867     -839     
- Misses       2031     3060    +1029

Impacted Files	Coverage Δ
pymc/gp/gp.py	`93.82% <ø> (ø)`
pymc/sampling/forward.py	`95.45% <ø> (ø)`
pymc/distributions/continuous.py	`65.45% <33.33%> (-32.24%)`	⬇️
pymc/testing.py	`86.10% <81.81%> (-5.45%)`	⬇️
pymc/distributions/discrete.py	`98.73% <100.00%> (+0.01%)`	⬆️

... and 42 files with indirect coverage changes

Also extended testing.check_icdf with skip_paradomain_outside_edge_test param --------- Co-authored-by: Michal Raczycki <[email protected]>

daniel-saunders-phil · 2023-03-20T15:55:22Z

@OriolAbril would you be willing to help me with this?

fonnesbeck · 2023-03-20T16:10:34Z

Thanks for taking this on!

reshamas · 2023-03-20T19:14:22Z

@daniel-saunders-phil
Good to see you here again!
an option:

upcoming sprint: https://www.meetup.com/data-umbrella/events/291768545/

…ctive (pymc-devs#6616) * Improved docstring for predictions argument in sample_posterior_predictive * Fix typos Co-authored-by: Ravin Kumar <[email protected]> --------- Co-authored-by: Ravin Kumar <[email protected]>

OriolAbril · 2023-03-23T08:53:43Z

If an argument is deprecated it should still be in the docs, with a note in the description about being deprecated. It is once it is eventually removed that both argument and itd docstring are deleted

For model you should use:

model : Model, optional
    Description, can be one sentence. It is here where conditions and further explanation om what optional means go. In the first line it is only argument name and type info.

Potential description "Model with the GP component for which predictions will be generated. It is optional when inside a with context, otherwise it is required"

OriolAbril

Quick review from my phone, not sure when I'll have time for a full review

OriolAbril · 2023-03-23T08:57:49Z

pymc/gp/gp.py

-        The covariance function.  Defaults to zero.
-    mean_func: None, instance of Mean
-        The mean function.  Defaults to zero.
+    cov_func : 2D array, or instance of Covariance, default Zero


Suggested change

cov_func : 2D array, or instance of Covariance, default Zero

cov_func : 2D array_like or Covariance, default ~pymc.gp.cov.Constant

Similar comment for mean_func, and I would also switch the order so it matches the order they appear in __init__

OriolAbril · 2023-03-23T09:10:18Z

pymc/gp/gp.py

+            vector with shape `(n, 1)`.
+        given : dict, optional
+            Can take as key value pairs: `X`, `y`,
+            and `gp`. See the section in the documentation on additive GP


This can't be a mention, it should be a cross-reference. Anyone knows what page it means?

After digging around for a while, it looks like the only illustration of how additive GPs work is https://www.pymc.io/projects/examples/en/latest/gaussian_processes/GP-MaunaLoa.html

It seems like a gap in the examples/docs and might need to be addressed in future work.

Maybe it's this page still pending update from v3? #5720 cc @bwengals @canyon289

Can we link to unfinished work in a way that the docs will sync up once #5720 is finished or should we leave this be for now?

Yeah it might be, it should point here: https://www.pymc.io/projects/docs/en/v3/Gaussian_Processes.html#additive-gps

OriolAbril · 2023-03-23T09:12:39Z

pymc/gp/gp.py

        **kwargs
-            Extra keyword arguments that are passed to distribution constructor.
+            Extra keyword arguments that are passed to 'MvStudentT' distribution constructor.


Suggested change

Extra keyword arguments that are passed to 'MvStudentT' distribution constructor.

Extra keyword arguments that are passed to :class:`~pymc.MvStudentT' distribution constructor.

OriolAbril · 2023-03-23T09:20:28Z

pymc/gp/gp.py

            Function input values.  If one-dimensional, must be a column
            vector with shape `(n, 1)`.
-        point: pymc.model.Point
+        point : pymc.model.Point, optional


Suggested change

point : pymc.model.Point, optional

point : pymc.Point, optional

daniel-saunders-phil · 2023-03-23T17:17:59Z

Hi Oriol, thanks really helpful! I'll do another pass on this in the next couple days and then submit a real PR.

… docstring

…, fixed

daniel-saunders-phil · 2023-03-29T03:39:08Z

Okay, I got this pr into a stable shape. It’s ready for review whenever anyone is up for it.

the only unresolved bit is the reference to the additive GP example but given that it is still underconstruction, I’ll leave it be.

OriolAbril

Some minor general comments:

I prefer using float/int (depending on which is correct) instead of scalar
I would also delete the "instance of Class" and leave "Class" only, I think the info conveyed by both is the same.
For the reference, I would add a
```
See the :ref:`section <additive_gp>` in the documentation on additive GP models ...
```
We can then comment on the issue to make sure that target is added when the doc is updated

daniel-saunders-phil · 2023-03-29T17:58:33Z

Thanks for taking a look. I adjusted those things and sent in a new commit.

OriolAbril

not sure what is going on with the extra modified files, so not merging yet. The docstrings look great already

daniel-saunders-phil · 2023-03-30T17:15:25Z

Oh I see what happened with the extra files: a couple weeks ago I messed up some rebasing command and unwittingly added 4 other people's commits to my own. Do you know the best way to proceed? I could open up a new clean PR with only the finished GP file on it. Or is there a way to delete those commits off my branch?

michaelosthege · 2023-04-01T09:48:16Z

Oh I see what happened with the extra files: a couple weeks ago I messed up some rebasing command and unwittingly added 4 other people's commits to my own. Do you know the best way to proceed? I could open up a new clean PR with only the finished GP file on it. Or is there a way to delete those commits off my branch?

You could try to rebase on latest main and drop the commits you want to remove, then force-push.

If that doesn't work just branch off main with a new branch, apply the GP docstring changes, then force-push to the old (this PR) branch.

…, fixed

This reverts commit 473c952.

…#6528)"" This reverts commit 86f36031759620a124ba5b46faf9305d2b160b71.

… docstring

daniel-saunders-phil · 2023-04-06T16:25:01Z

Thanks Michael for the suggestions. I tried both routes and ran into trouble. After chatting with Oriol, we decided the best route was to just create a new branch and new PR #6652. We successfully merge it a little bit ago. I appreciate the help everyone!

overhaul gp docstrings

03a0267

NathanielF and others added 2 commits March 18, 2023 09:20

Fix a typo in the docstring of OderedLogistic (pymc-devs#6611)

67925df

Implement icdf for Univariate distribution (pymc-devs#6528)

473c952

Also extended testing.check_icdf with skip_paradomain_outside_edge_test param --------- Co-authored-by: Michal Raczycki <[email protected]>

OriolAbril reviewed Mar 23, 2023

View reviewed changes

gokuld and others added 4 commits March 24, 2023 07:32

Added ICDF for the discrete uniform distribution.

2fcce43

overhaul gp docstrings

bb4e7a1

revised updates to gp docstrings

40d4c30

Merge branch 'docstring' of github.com:daniel-saunders-phil/pymc into…

a24b74b

… docstring

daniel-saunders-phil marked this pull request as ready for review March 24, 2023 21:08

daniel-saunders-phil marked this pull request as draft March 24, 2023 21:14

daniel-saunders-phil added 3 commits March 24, 2023 14:24

correcting errors introduced in last push

2a62c0e

second attempt to fix duplication errors

b94ba08

references to pymc.mvstudentt and similar weren't rendering correctly…

48a0292

…, fixed

daniel-saunders-phil marked this pull request as ready for review March 25, 2023 19:00

OriolAbril reviewed Mar 29, 2023

View reviewed changes

implementing Oriol's suggestions

955d4a9

OriolAbril approved these changes Mar 30, 2023

View reviewed changes

daniel-saunders-phil added 2 commits April 6, 2023 07:14

second attempt to fix duplication errors

e09c17a

references to pymc.mvstudentt and similar weren't rendering correctly…

031ee71

…, fixed

daniel-saunders-phil added 4 commits April 6, 2023 07:14

implementing Oriol's suggestions

b506171

Revert "Implement icdf for Univariate distribution (pymc-devs#6528)"

b528822

This reverts commit 473c952.

Revert "Revert "Implement icdf for Univariate distribution (pymc-devs…

efa315c

…#6528)"" This reverts commit 86f36031759620a124ba5b46faf9305d2b160b71.

Merge branch 'docstring' of github.com:daniel-saunders-phil/pymc into…

8cdc77c

… docstring

daniel-saunders-phil mentioned this pull request Apr 6, 2023

Improve docstrings in GP module #6652

Merged

OriolAbril closed this Apr 6, 2023

daniel-saunders-phil deleted the docstring branch April 6, 2023 16:23

	cov_func : 2D array, or instance of Covariance, default Zero
	cov_func : 2D array_like or Covariance, default ~pymc.gp.cov.Constant

	Extra keyword arguments that are passed to 'MvStudentT' distribution constructor.
	Extra keyword arguments that are passed to :class:`~pymc.MvStudentT' distribution constructor.

	point : pymc.model.Point, optional
	point : pymc.Point, optional

Uh oh!

overhaul gp docstrings #6609

overhaul gp docstrings #6609

Uh oh!

Conversation

daniel-saunders-phil commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

daniel-saunders-phil commented Mar 20, 2023

Uh oh!

fonnesbeck commented Mar 20, 2023

Uh oh!

reshamas commented Mar 20, 2023

Uh oh!

OriolAbril commented Mar 23, 2023

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-saunders-phil commented Mar 23, 2023

Uh oh!

daniel-saunders-phil commented Mar 29, 2023

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

daniel-saunders-phil commented Mar 29, 2023

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

daniel-saunders-phil commented Mar 30, 2023

Uh oh!

michaelosthege commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daniel-saunders-phil commented Apr 6, 2023

Uh oh!

Uh oh!

daniel-saunders-phil commented Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

michaelosthege commented Apr 1, 2023 •

edited

Loading