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

Add helper wrapper aound Interval transform #5347

Merged
merged 3 commits into from
Mar 18, 2022

Conversation

ricardoV94
Copy link
Member

Thank your for opening a PR!

Depending on what your PR does, here are a few things you might want to address in the description:

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #5347 (d0d5929) into main (a0cff37) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5347      +/-   ##
==========================================
+ Coverage   87.56%   87.66%   +0.10%     
==========================================
  Files          76       76              
  Lines       13732    13714      -18     
==========================================
- Hits        12025    12023       -2     
+ Misses       1707     1691      -16     
Impacted Files Coverage Δ
pymc/__init__.py 100.00% <ø> (ø)
pymc/distributions/continuous.py 98.07% <100.00%> (+0.84%) ⬆️
pymc/distributions/multivariate.py 92.18% <100.00%> (ø)
pymc/distributions/transforms.py 100.00% <100.00%> (ø)
pymc/model.py 86.00% <0.00%> (+0.05%) ⬆️
pymc/distributions/timeseries.py 22.89% <0.00%> (+0.53%) ⬆️
pymc/distributions/mixture.py 94.73% <0.00%> (+1.59%) ⬆️

@ricardoV94
Copy link
Member Author

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jan 13, 2022

CC @OriolAbril guru 🧞

@OriolAbril
Copy link
Member

There are two issues here. The main one is that we are trying to use pymc.transforms. This is what happens on my computer:

In [1]: import pymc.transforms as transforms
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-1-dd33c3c1e58a> in <module>
----> 1 import pymc.transforms as transforms

ModuleNotFoundError: No module named 'pymc.transforms'

In [2]: from pymc import transforms

In [3]: transforms.Chain
Out[3]: pymc.distributions.transforms.Chain

In [4]: import pymc.transforms
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-4-2bd367763092> in <module>
----> 1 import pymc.transforms

ModuleNotFoundError: No module named 'pymc.transforms'

I have no idea how this happens nor how to fix it.

sphinx uses import pymc.transforms to then get the docstrings form the onbjects listed in https://github.com/pymc-devs/pymc/blob/main/docs/source/api/distributions/transforms.rst. As it fails, only the index table at https://pymc--5347.org.readthedocs.build/en/5347/api/distributions/transforms.html is generated, but unlike what happens with continuous distributions for example, you can't click on them to get to the specific page with the whole docstring.

The second much less important issue is that even if that were fixed, docstrings have some constraints because we use numpydoc to parse them, so directives right in the extended description might not work as expected. I'd suggest using sections
defined by numpydoc instead of raw directives, iirc, the warning one is parsed and formatted as an admonition, but the notes are not. Either way, numpydoc also defines the order in which every section appears in the rendered docs independently of the order in the docstring I belive, so if the notes end up at the bottom and we want the tip to appear at the top we can look into it.

@OriolAbril
Copy link
Member

@ricardoV94
Copy link
Member Author

@OriolAbril I fixed the module path and the elements are now clickable. It still seems like sphinx ignores the custom __doc__:

https://pymc--5347.org.readthedocs.build/en/5347/api/distributions/generated/pymc.distributions.transforms.Interval.html#pymc.distributions.transforms.Interval

Would it be better to add specific documentation directly in the rst (shivers) file?

@OriolAbril
Copy link
Member

OriolAbril commented Jan 14, 2022

It is still being imported in init https://github.com/ricardoV94/pymc3/blob/transform_issue/pymc/__init__.py#L49 to expose pymc.transforms to users, and even though it is now documented as pymc.distributions.transforms, the examples (and cross references) use pymc.transforms instead. I find this very confusing. Imo, If we want users to use pymc.transforms we should document there and try and fix that import issue. The transforms file seems quite standalone, maybe it could be moved outside the distributions folder? (even then we can document it inside distributions in the api, those groupings are conceptual now, most things are imported and documented as pymc.object)

The ignoring of the doc is very confusing. The rest are working, my guess is that somehow doing this trick on classes instead of functions is not possible or has to be done in some other way. And the worse thing is it has a docstring somehow. Where does the alias of aeppl.transforms.intervaltransform come from? It even uses the correct syntax for sphinx cross references?!?!?!!!!

Would it be better to add specific documentation directly in the rst (shivers) file?

We can try that yeah, but then the docstring won't be available from ides which is far from ideal.

@ricardoV94 ricardoV94 marked this pull request as draft January 21, 2022 12:22
@OriolAbril
Copy link
Member

OriolAbril commented Jan 25, 2022

had an idea that might fix this? (coming from someone with little understanding of imports)

We now do

from pymc.distributions import transforms

in the "global" init, but we also do

from pymc.distributions import *

and all distributions are available in the pymc namespace. So maybe doing the from pymc.distributions import transforms in the distributions/__init__.py and adding transforms to the __all__ variable will work if we then keep only the from distributions import * in the global init

(assuming we do want to keep the transforms inside the distributions folder)

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jan 28, 2022

@OriolAbril sounds like it could work

@ricardoV94 ricardoV94 force-pushed the transform_issue branch 2 times, most recently from 99dc73a to 521df93 Compare March 17, 2022 08:54
@ricardoV94 ricardoV94 marked this pull request as ready for review March 17, 2022 08:54
@ricardoV94 ricardoV94 changed the title Update docstrings of Interval transform Add helper wrapper aound Interval transform Mar 17, 2022
@ricardoV94
Copy link
Member Author

@OriolAbril does my last commit do what you were suggesting?

@ricardoV94
Copy link
Member Author

@OriolAbril
Copy link
Member

That was my idea but transforms continues to not be a proper module:

In [1]: import pymc.transforms as transforms
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
Input In [1], in <module>
----> 1 import pymc.transforms as transforms

ModuleNotFoundError: No module named 'pymc.transforms'

In [2]: from pymc import transforms

In [3]:

@OriolAbril
Copy link
Member

I tried updating the transforms.rst file to change currentmodule:: pymc.transforms by pymc only, then list elements as transforms.simplex... instead of only simplex. This partially tricks sphinx, it generates pages for each object, but is still unable to access their __doc__ attribute and the page ends up empty and not properly linked:

imatge

I think we can merge for now and at some point fix the import issue so that both import and from ... import work. If that is fixed, the docs will render correctly without any change to the docs themselves.

@ricardoV94
Copy link
Member Author

@OriolAbril would it help if transforms was a proper module itself? With a directory and __init__.py?

@OriolAbril
Copy link
Member

OriolAbril commented Mar 18, 2022

I am quite sure it will. Sphinx works as long as it is able to import the objects. If both imports from #5347 (comment) work then sphinx will work, I think making it a proper module will make the two import ways work, but here is where I am not completely sure.

@ricardoV94
Copy link
Member Author

I'll give it a try

@ricardoV94 ricardoV94 force-pushed the transform_issue branch 2 times, most recently from cb91bef to 3a184e7 Compare March 18, 2022 12:00
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. My only comment is this probably deserves a mention in the release notes

@OriolAbril
Copy link
Member

Docs are working now: https://pymc--5347.org.readthedocs.build/en/5347/api/distributions/transforms.html. There are some rendering issues though.

@ricardoV94 ricardoV94 force-pushed the transform_issue branch 4 times, most recently from cd32e02 to aed8180 Compare March 18, 2022 12:48
@ricardoV94
Copy link
Member Author

@OriolAbril I think formatting is fixed now?

@OriolAbril
Copy link
Member

yep

@ricardoV94 ricardoV94 merged commit 02860d3 into pymc-devs:main Mar 18, 2022
@ricardoV94
Copy link
Member Author

Thanks a lot for the help @OriolAbril. I realized now I should have added you as a co-author :/

@ricardoV94 ricardoV94 deleted the transform_issue branch March 18, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants