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

Create helper pm.draw() to take draws for a given variable #5340

Merged
merged 14 commits into from
Jan 13, 2022

Conversation

danhphan
Copy link
Member

@danhphan danhphan commented Jan 11, 2022

Hi @ricardoV94

I have created helper pm.draw() to take draws for a given variable or several variables, to address the issue #5311
The draw function is stored in pymc/sampling.py along with docstring and examples.
The test cases are added in class TestDraw in pymc/tests/tests_sampling.py

Please let me know if there is anything needed to update.

Thank you.

Closes #5311

pymc/sampling.py Outdated Show resolved Hide resolved
pymc/sampling.py Outdated Show resolved Hide resolved
pymc/sampling.py Outdated Show resolved Hide resolved
pymc/sampling.py Outdated Show resolved Hide resolved
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.

Looks great! I left a couple of suggestions above, let me know if you have any questions

@ricardoV94 ricardoV94 added this to the v4.0.0b3 milestone Jan 11, 2022
@danhphan
Copy link
Member Author

By the ways, after updating the draw function and test cases, how should I proceed? Should I make a new Pull request, or how to integrate the updated codes into this Pull request? Many thanks.

@ricardoV94
Copy link
Member

By the ways, after updating the draw function and test cases, how should I proceed? Should I make a new Pull request, or how to integrate the updated codes into this Pull request? Many thanks.

You can just add more commits and push to your GitHub branch, they will show up automatically here in the PR

@danhphan
Copy link
Member Author

Hi yes, I have updated the codes and push it to the GitHub. Thanks.

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.

Great progress! I left a couple more small comments/suggestions, but it looks pretty close to done.

You will also need to update the Release Notes to mention the new feature, and in these lines say that pm.draw(x) shoud be used instead of x.eval():

- `pm.Distribution(...).random()` is now `pm.Distribution(...).eval()`

Let me know if you have any questions!

pymc/sampling.py Outdated Show resolved Hide resolved
@ricardoV94
Copy link
Member

ricardoV94 commented Jan 13, 2022

@OriolAbril It seems like the code block in the function docstrings is not rendering in the docs preview. Is there a formatting issue or is this expected?

https://pymc--5340.org.readthedocs.build/en/5340/api/samplers.html#pymc.sampling.draw

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #5340 (632f1a1) into main (f6b930b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5340      +/-   ##
==========================================
+ Coverage   80.19%   80.23%   +0.03%     
==========================================
  Files          89       89              
  Lines       14847    14856       +9     
==========================================
+ Hits        11907    11919      +12     
+ Misses       2940     2937       -3     
Impacted Files Coverage Δ
pymc/aesaraf.py 90.20% <100.00%> (ø)
pymc/sampling.py 86.41% <100.00%> (+0.14%) ⬆️
pymc/parallel_sampling.py 87.70% <0.00%> (+0.99%) ⬆️

pymc/sampling.py Outdated Show resolved Hide resolved
pymc/sampling.py Outdated Show resolved Hide resolved
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.

Trying to keep the Release Notes a bit more brief, as they are going to be pretty long for the V4 release already

RELEASE-NOTES.md Outdated Show resolved Hide resolved
RELEASE-NOTES.md Outdated Show resolved Hide resolved
RELEASE-NOTES.md Outdated Show resolved Hide resolved
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.

Looks great! Will merge after the tests pass

@ricardoV94
Copy link
Member

@danhphan I fixed the branch conflicts and tweaked some type hints and internal variable names, but nothing more. If you need to make further changes in your PR you will have to rebase from here (your origin)

@ricardoV94 ricardoV94 modified the milestones: v4.0.0b3, v4.0.0b2 Jan 13, 2022
@danhphan
Copy link
Member Author

Hi yes, thanks a lot for your help 👍 @ricardoV94

@ricardoV94 ricardoV94 merged commit 1b32070 into pymc-devs:main Jan 13, 2022
@ricardoV94
Copy link
Member

@danhphan Ended up doing more changes than I expected, so sorry for robbing the fun from you...

Thanks a lot and congrats for the PR!

@twiecki
Copy link
Member

twiecki commented Jan 13, 2022

Indeed, this is one hell of a first PR @danhphan!

@danhphan
Copy link
Member Author

Hi, all good @ricardoV94 @twiecki, It is great to contribute and learn as well :)

@danhphan danhphan deleted the pm-draw branch February 24, 2022 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create helper pm.draw() to take draws from a given variable
4 participants