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 introduction section to Transformations API page to document role of Transforms #7232

Merged
merged 4 commits into from
Apr 19, 2024

Conversation

mkusnetsov
Copy link
Contributor

@mkusnetsov mkusnetsov commented Mar 29, 2024

Description

This PR adds an introduction section to the Transformations API page to:

  • explain the principal purpose of transforms
  • warn that most users don't need to play with these and that most attempts to do so don't do what users tend to think
  • provide some guidance on when tweaking transforms may be appropriate and what other options user have when wanting to deal with transformed random variables

In addition this PR:

  • removes a seemingly incorrect deprecation comment next to the ordered transform (I think it was accidentally left over from when univariate_ordered was deprecated)
  • removes mention of SumsTo1, which is being deprecated
  • organises instance and class names in the autosummary to
    1. alphebtize them, and
    2. add missing class names for the included instances

This PR assumes that the issue #5674 is closed (via PR #7207) to avoid extensive (and somewhat non-intuitive to casual user) guidance that will shortly be outdated

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc--7232.org.readthedocs.build/en/7232/

Copy link

welcome bot commented Mar 29, 2024

Thank You Banner]
💖 Thanks for opening this pull request! 💖 The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.

@mkusnetsov mkusnetsov marked this pull request as ready for review March 29, 2024 23:59
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

This explanation is absolutely incredible!

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Just a small suggestion, as you mentioned we will have both transform and default_transform after: #7207

All you said about default_transform is correct. When users want to add other constraints but not modify the default one, they should probably use the transform kwarg. Under the hood, we will build a ChainTransform, that applies the user provided transform on top of the default one.

@mkusnetsov
Copy link
Contributor Author

Thanks, @ricardoV94! That's a good point - I pushed some changes to clarify transform kwarg as distinct from default_transform.

But I might be doing something wrong? Although I can see the commits and the changes, the preview documentation is still showing the original version, not the revised one... (I know it takes a while to re-build but it seems to be saying "Read the Docs build succeeded!")

@ricardoV94
Copy link
Member

But I might be doing something wrong? Although I can see the commits and the changes, the preview documentation is still showing the original version, not the revised one... (I know it takes a while to re-build but it seems to be saying "Read the Docs build succeeded!")

It takes some time for the preview to update. I think it's correct now? https://pymcio--7232.org.readthedocs.build/projects/docs/en/7232/api/distributions/transforms.html

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

I think this is perfect as is. I would just wait for the related PR to be merged, so that our docs are not "ahead of time"

@ricardoV94 ricardoV94 marked this pull request as draft April 3, 2024 07:28
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.30%. Comparing base (4f6831e) to head (f29112f).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7232   +/-   ##
=======================================
  Coverage   92.30%   92.30%           
=======================================
  Files         100      100           
  Lines       16895    16895           
=======================================
  Hits        15595    15595           
  Misses       1300     1300           
Files Coverage Δ
pymc/distributions/transforms.py 98.47% <ø> (ø)

@mkusnetsov
Copy link
Contributor Author

Thank you for the kind words! It is correct now, yes. Will know that it takes a while, in the future. And yes - it absolutely makes sense to wait for the other PR.

I left some open questions on the underlying issue. Are any of them worth pursuing either in a separate issue or a discussion? Just checking because I assume once that issue is closed they might get lost.

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.

I review docs related PRs quite sparsely if I have time and after seeing this one I wanted to add my praise too at use of rST, both at syntax level and conceptual level 😄 📚

Also, in case someone wonders, the fact that references to pymc.sample, pymc.sample_prior_predictive... are not (yet) rendered as links in the rtd preview is an issue with how those are documented, once that is fixed all cross-references here will work

@ricardoV94 ricardoV94 marked this pull request as ready for review April 19, 2024 07:24
@ricardoV94 ricardoV94 merged commit ed62da1 into pymc-devs:main Apr 19, 2024
23 checks passed
Copy link

welcome bot commented Apr 19, 2024

Congratulations Banner]
Congrats on merging your first pull request! 🎉 We here at PyMC are proud of you! 💖 Thank you so much for your contribution 🎁

@ricardoV94
Copy link
Member

Thanks a ton @mkusnetsov, don't hesitate in opening issues/PRs for other areas you find documentation to be lacking

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.

Document the role of transforms
3 participants