Get the transform docs into the API pages. #3690

rpgoldman · 2019-11-22T20:24:39Z

I'm marking this as WIP, because the entries for the transform instances in the docs are not formatted as well as I would like.

Someone should also vet my additional exports. I found that the transforms were not being exported from distributions into pymc3 -- some of them were dropped and only exported from distributions.

If we want to have a hierarchical namespace like this, we should probably spend a little time deciding how it will be laid out.

The doc strings for the transforms were not linked into the documentation page for distributions.

ColCarroll · 2019-11-22T20:36:48Z

pymc3/distributions/__init__.py

-from .transforms import logodds
-from .transforms import log
-from .transforms import sum_to_1
+from .transforms import *


I like this better than the current method. Two things:

This is technically a breaking change, since pm.distributions.transform will no longer work. I bet this messes up 0 people, but we should add a deprecation warning before removing.

As a suggestion, I think we should surface transforms as pm.transforms, so that you could use pm.transforms.ordered() instead of pm.distributions.ordered() (In fact, I think right now pm.log might be a transform, and not a logarithm?). I think we would do this by not importing anything from .transforms here, but in the top level init, import .distributions.transforms as transforms. How does that sound?

WRT your (1), I think we could just not export transform, since it's just an alias for Transform. That's also theoretically a breaking change, but seems even less likely to cause troubles. Especially since one should not use transform as the name of a class in python.

(2) Sounds good to me.

LMK what you think about my responses and I will make the changes in the PR, or I believe you could do them yourself, if that's what you prefer.

Both of those sound good to me. Feel free to ping after you mess with those imports I'll kick the tires locally (and add tests if I find anything :) ) and then merge. Ooh, I'll check the doc thing you just mentioned, too.

Wait -- why is pm.distributions.transform no longer going to work? It's in the __all__ list in transforms.py, so I think it's still ok, isn't it?
But I like the idea of deprecating it, since for a class Transform is preferable. Also transform, not being an initialized entity, doesn't serve the same purpose as the other lower-case names.

rpgoldman · 2019-11-22T20:42:19Z

When I look at the formatted docs for the transforms, the parameters are displayed like this

**foo**type

instead of

***foo** : type

as they should. Please have a look and see if this works for you -- it could be just a local fail, in which case we can ignore it.

I really don't know what to do about what sphinx is doing with the ::autodata::.

rpgoldman · 2019-11-22T20:44:38Z

Actually, my autosummary also seems broken.

The transforms code has lower-case alternatives to the upper-cased class names. Sometimes these are instances of the classes (e.g., `log` is an instance of the `Log` transform), but sometimes they are aliases (e.g., `lowerbound` is an alias for `LowerBound`). I couldn't find a good way to document these in the source file, so they are hand-documented in transforms.rst.

codecov · 2019-11-24T20:29:44Z

Codecov Report

Merging #3690 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3690      +/-   ##
==========================================
+ Coverage    89.9%   89.91%   +<.01%     
==========================================
  Files         134      134              
  Lines       20184    20189       +5     
==========================================
+ Hits        18147    18152       +5     
  Misses       2037     2037

Impacted Files	Coverage Δ
pymc3/distributions/continuous.py	`80.85% <ø> (ø)`	⬆️
pymc3/distributions/__init__.py	`100% <ø> (ø)`	⬆️
pymc3/distributions/transforms.py	`87.65% <100%> (ø)`	⬆️
pymc3/sampling.py	`83.09% <0%> (ø)`	⬆️
pymc3/tests/test_sampling.py	`99.54% <0%> (+0.01%)`	⬆️

docs/source/developer_guide.rst

Thomas caught a typo of mine, and when fixing it, I picked off some grammar errors and improved some typesetting.

twiecki · 2019-11-25T15:49:56Z

Did you compile things locally to ensure everything looks right?

twiecki · 2019-11-25T15:50:22Z

And needs a line in release-notes.

rpgoldman · 2019-11-25T16:02:25Z

Did you compile things locally to ensure everything looks right?

I did, but .... they don't. See my discussion with @ColCarroll above:

The autosummary doesn't work for me. This is probably at least partly due to my hand-documenting the transform aliases that are exported. I do not know how to fix this. If anyone else does, that would be great. Otherwise, I can hand-hack a toctree here.
Parameters are not properly typeset for me. I have seen this before, but don't know what causes it. I don't know whether this is a problem for others, or something wrong with my sphinx set up. For me it seems to be all over the PyMC3 docs, not just here. Here's an example from the distributions api pages:

rpgoldman · 2019-11-25T16:05:40Z

And needs a line in release-notes.

Done!

Fix typesetting of parameters in the docstrings.

@aloctavodia

Thanks to @aloctavodia for help.

I still don't know how to get the title of *this* page *not* to appear in its table of contents. Unfortunately, `autosummary` does not work at all.

While I was at it, fixed the punctuation of parameters in the docstrings so that Sphinx will format them correctly.

rpgoldman · 2019-11-26T17:24:29Z

I've taken this as far as I can go with it. .. autosummary:: doesn't work because of something to do with the aliases that I had to document by hand (couldn't figure out how to make Sphinx do that properly).

I'm going to take off the "WIP" -- @ColCarroll will you please have one last look and then squash merge?

ColCarroll · 2019-11-26T17:57:39Z

Built docs here -- I found the transform docs, but would love if you confirm they're what you expected!

https://pymc3.colindcarroll.com/

rpgoldman · 2019-11-26T18:21:45Z

That looks as I had expected, thanks, @ColCarroll

ColCarroll · 2019-11-26T18:41:37Z

thanks @rpgoldman -- careful and valuable work as always.

rpgoldman · 2019-11-26T19:02:48Z

Thanks for the kind words -- couldn't have done it with the help of a bunch of the devs' kind help.

rpgoldman added 2 commits November 22, 2019 14:20

Extend exports from transforms.

eec41a9

Add docs for distributions.transforms.

e657790

The doc strings for the transforms were not linked into the documentation page for distributions.

rpgoldman requested a review from ColCarroll November 22, 2019 20:24

ColCarroll reviewed Nov 22, 2019

View reviewed changes

rpgoldman added 2 commits November 24, 2019 14:25

Export improvements requested by @ColCarroll.

859dac5

rpgoldman added 2 commits November 24, 2019 15:22

Minor edits to clarify transformations.

75f4f58

Added API page for random variables.

76da1b4

twiecki reviewed Nov 25, 2019

View reviewed changes

docs/source/developer_guide.rst Outdated Show resolved Hide resolved

Fix typo @twiecki caught and some others.

62a7e1a

Thomas caught a typo of mine, and when fixing it, I picked off some grammar errors and improved some typesetting.

Add to release notes.

e8dfa94

rpgoldman added 4 commits November 25, 2019 15:46

Fix typesetting of parameters.

4cfa494

Fix typesetting of parameters in the docstrings.

Add docstring for t_stick_breaking()

4220414

Thanks to @aloctavodia for help.

Slightly improved Table of Contents.

879fa5d

I still don't know how to get the title of *this* page *not* to appear in its table of contents. Unfortunately, `autosummary` does not work at all.

Fix formatting of additional parameters.

0556809

While I was at it, fixed the punctuation of parameters in the docstrings so that Sphinx will format them correctly.

rpgoldman changed the title ~~WIP: Get the transform docs into the API pages.~~ Get the transform docs into the API pages. Nov 26, 2019

ColCarroll merged commit 9c4b740 into pymc-devs:master Nov 26, 2019

rpgoldman deleted the transform-docs branch November 26, 2019 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get the transform docs into the API pages. #3690

Get the transform docs into the API pages. #3690

rpgoldman commented Nov 22, 2019

ColCarroll Nov 22, 2019

rpgoldman Nov 22, 2019

ColCarroll Nov 22, 2019

rpgoldman Nov 22, 2019

rpgoldman commented Nov 22, 2019

rpgoldman commented Nov 22, 2019

codecov bot commented Nov 24, 2019 •

edited

Loading

twiecki commented Nov 25, 2019

twiecki commented Nov 25, 2019

rpgoldman commented Nov 25, 2019

rpgoldman commented Nov 25, 2019

rpgoldman commented Nov 26, 2019

ColCarroll commented Nov 26, 2019

rpgoldman commented Nov 26, 2019

ColCarroll commented Nov 26, 2019

rpgoldman commented Nov 26, 2019

Get the transform docs into the API pages. #3690

Get the transform docs into the API pages. #3690

Conversation

rpgoldman commented Nov 22, 2019

ColCarroll Nov 22, 2019

Choose a reason for hiding this comment

rpgoldman Nov 22, 2019

Choose a reason for hiding this comment

ColCarroll Nov 22, 2019

Choose a reason for hiding this comment

rpgoldman Nov 22, 2019

Choose a reason for hiding this comment

rpgoldman commented Nov 22, 2019

rpgoldman commented Nov 22, 2019

codecov bot commented Nov 24, 2019 • edited Loading

Codecov Report

twiecki commented Nov 25, 2019

twiecki commented Nov 25, 2019

rpgoldman commented Nov 25, 2019

rpgoldman commented Nov 25, 2019

rpgoldman commented Nov 26, 2019

ColCarroll commented Nov 26, 2019

rpgoldman commented Nov 26, 2019

ColCarroll commented Nov 26, 2019

rpgoldman commented Nov 26, 2019

codecov bot commented Nov 24, 2019 •

edited

Loading