-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
15b7355
to
edff5b3
Compare
5ef4bd5
to
1fbe6b8
Compare
Codecov Report
@@ 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
|
The doc trick does not seem to work: https://pymc--5347.org.readthedocs.build/en/5347/api/distributions/transforms.html |
CC @OriolAbril guru 🧞 |
1fbe6b8
to
1679ca2
Compare
There are two issues here. The main one is that we are trying to use 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 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 |
@OriolAbril I fixed the module path and the elements are now clickable. It still seems like sphinx ignores the custom Would it be better to add specific documentation directly in the |
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
We can try that yeah, but then the docstring won't be available from ides which is far from ideal. |
had an idea that might fix this? (coming from someone with little understanding of imports) We now do Line 54 in eb5177a
in the "global" init, but we also do Line 53 in eb5177a
and all distributions are available in the pymc namespace. So maybe doing the (assuming we do want to keep the transforms inside the distributions folder) |
@OriolAbril sounds like it could work |
99dc73a
to
521df93
Compare
@OriolAbril does my last commit do what you were suggesting? |
439deb2
to
463f885
Compare
That was my idea but transforms continues to not be a proper module:
|
I tried updating the transforms.rst file to change I think we can merge for now and at some point fix the import issue so that both |
@OriolAbril would it help if |
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. |
I'll give it a try |
463f885
to
65d08ac
Compare
cb91bef
to
3a184e7
Compare
There was a problem hiding this 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
Docs are working now: https://pymc--5347.org.readthedocs.build/en/5347/api/distributions/transforms.html. There are some rendering issues though. |
cd32e02
to
aed8180
Compare
aed8180
to
d0d5929
Compare
@OriolAbril I think formatting is fixed now? |
yep |
Thanks a lot for the help @OriolAbril. I realized now I should have added you as a co-author :/ |
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: