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 V4 distribution implementation developer guide #4783

Merged
merged 7 commits into from
Jul 1, 2021

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jun 18, 2021

This PR adds a developer guide on how to implement (and test) new distributions for PyMC3 v4.

Somethings are still marked as TODO and feedback is much appreciated.

The way distributions are implemented (and tested) will probably evolve over time, and hopefully this document will be easy to update as those things happen.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 18, 2021

@OriolAbril I need quite some help to figure out how to properly integrate this with the rest of the docs, as well things related to markdown formatting, and cross references! :) Thanks in advance

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 18, 2021

@DRabbit17, can you have a look at the section on "Adding tests for the new RandomVariable", to see if I missed anything obvious, or said something wrong?

@ricardoV94 ricardoV94 force-pushed the distributions_guide branch 2 times, most recently from 9779dc6 to fe115b8 Compare June 18, 2021 10:35
@matteo-pallini
Copy link
Contributor

@DRabbit17, can you have a look at the section on "Adding tests for the new RandomVariable"

Looks pretty good to me

@OriolAbril
Copy link
Member

Also forgot to mention that this page will now be orphan, and not discoverable in the docs unless one searches for it. Not sure how but it should be added to the toctree or included within he developer guide

@ricardoV94
Copy link
Member Author

Also forgot to mention that this page will now be orphan, and not discoverable in the docs unless one searches for it. Not sure how but it should be added to the toctree or included within he developer guide

The developer guide should probably be organized hierarchically, instead of being a single file like it is now, no?

@OriolAbril
Copy link
Member

That would probably be best, so it can also include the architecture doc, style guides...

@ricardoV94 ricardoV94 force-pushed the distributions_guide branch from fe115b8 to f6c644f Compare June 21, 2021 08:43
@ricardoV94
Copy link
Member Author

I updated the references to follow the format you mentioned {class}``import. I added some terms that would go into the glossary, let me know how I can actually add the descriptions.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 21, 2021

That would probably be best, so it can also include the architecture doc, style guides...

I would defer that to a separate PR, and in the meantime just leave this guide hanging.

@ricardoV94 ricardoV94 force-pushed the distributions_guide branch from f6c644f to 54be453 Compare June 21, 2021 08:51
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Looks almost complete to me!
Let's address the ToDos and merge it sooner than later.

@ricardoV94 ricardoV94 marked this pull request as ready for review June 23, 2021 15:03

This guide provides an overview on how to implement a distribution for version 4 of PyMC3.
It is designed for developers who wish to add a new distribution to the library.
Users will not be aware of all this complexity and should instead make use of helper methods such as (TODO).
Copy link
Member

Choose a reason for hiding this comment

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

Leftover TODO

Copy link
Member Author

@ricardoV94 ricardoV94 Jun 23, 2021

Choose a reason for hiding this comment

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

It's still a pymc3 ToDo. This was the role of Densitydist but that was deprecated or at least not refactored for V4. Maybe we can link to something like: pymc-devs/pymc-examples#185

@zoj613
Copy link
Contributor

zoj613 commented Jun 23, 2021

I think adding information about extending Op for new distributions would be valuable. Aesara contains little to no information about this. The example at https://aesara.readthedocs.io/en/latest/extending/extending_aesara.html#example-op-definition is unhelpful and seems very incomplete, nor does it ever touch on the "optional" methods. Moreover, details about dealing with shapes is completely missing. It has been quite frustrating for me to plough through multiple pages of the docs and still not get it.

@ricardoV94
Copy link
Member Author

I think adding information about extending Op for new distributions would be valuable. Aesara contains little to no information about this. The example at https://aesara.readthedocs.io/en/latest/extending/extending_aesara.html#example-op-definition is unhelpful and seems very incomplete, nor does it ever touch on the "optional" methods. Moreover, details about dealing with shapes is completely missing. It has been quite frustrating for me to plough through multiple pages of the docs and still not get it.

Do you want to open a Github issue for this? I think it could be important, but perhaps out of scope for this guide.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 25, 2021

The TODO may point to pymc-devs/pymc-examples#185 in the future, but that is still an early WIP. I think it's fine to leave it for the time-being. Similarly, the glossary will not work until #4797 is wrapped up.

I am okay merging this and leaving it as an orphan-page until the "dev-branch" that @martinacantaro has suggested is created. If not, we can leave this as a draft PR. As you guys see fit.

@ricardoV94 ricardoV94 force-pushed the distributions_guide branch from 97725cd to d747802 Compare June 30, 2021 07:13
@michaelosthege michaelosthege merged commit ef97da3 into pymc-devs:main Jul 1, 2021
@ricardoV94 ricardoV94 deleted the distributions_guide branch July 5, 2021 08:52
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.

5 participants