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

Update docstrings for sample_smc and smc.py #6114

Merged
merged 12 commits into from
Nov 19, 2022

Conversation

rowan-schaefer
Copy link
Contributor

What is this PR about?
#DataUmbrellaPyMCSprint
References #5459

  • Docstring formatting
  • Some typos
  • Added in kwargs for IMH and MH
  • Unsure about line “Model (optional if in with context)” but I saw this was still present on several other pages that had already been edited
  • I think there may be more in here that are optional?
  • For random_seed (int, array_like of int, RandomState or Generator, optional) unsure if you want curly bracket format like in numpydoc - didn’t see that used anywhere else in pymc docs
    i.e. in NumPy docs
    seed{None, int, array_like[ints], SeedSequence, BitGenerator, Generator}, optional
  • Was unable to get sphinx preview working when using virtual env so I hope this is ok

Checklist

Major / Breaking Changes

  • ...

Bugfixes / New features

  • ...

Docs / Maintenance

  • ...

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #6114 (9b72724) into main (4acd98e) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6114   +/-   ##
=======================================
  Coverage   94.17%   94.17%           
=======================================
  Files         111      111           
  Lines       23908    23908           
=======================================
  Hits        22515    22515           
  Misses       1393     1393           
Impacted Files Coverage Δ
pymc/smc/kernels.py 97.41% <ø> (ø)
pymc/smc/sampling.py 86.42% <ø> (ø)
pymc/variational/inference.py 84.97% <ø> (ø)

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.

Thanks for the PR, I hope the comments and answers help

pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/smc.py Outdated Show resolved Hide resolved
@cluhmann
Copy link
Member

This is a pretty meaty docstring. Thanks for the contribution @rowangayleschaefer and hopefully the build error gets fixed soon!

@OriolAbril
Copy link
Member

hopefully the build error gets fixed soon!

I merged the PR that fix this less than a minute ago!

@rowan-schaefer
Copy link
Contributor Author

Thank you! Should I make another PR to amend the notes that you made?

@reshamas
Copy link
Contributor

Thank you! Should I make another PR to amend the notes that you made?

Hi @rowangayleschaefer
You should be able to accept the changes that @OriolAbril suggested.
Then, you can pull those changes down locally with git pull origin rowan_5459

@cluhmann @OriolAbril
Feel free to correct me if I am thinking of something else.

@rowan-schaefer
Copy link
Contributor Author

Done!

@reshamas reshamas requested a review from OriolAbril September 13, 2022 22:22
@OriolAbril
Copy link
Member

@rowangayleschaefer you should rebase your branch to make sure it contains all the changes from main (and therefore includes the fix for the docs), otherwise we won't get the PR docs preview.

You can do that with the following commands:

git fetch origin  # make sure local git is aware of latest changes in the fork
git fetch upstream  # idem, but for main pymc repo
git checkout rowan_5459  # make sure you are on the right branch
git pull  # get the changes from the fork (the updates from my suggestions)
git rebase upstream/main  # get the changes from pymc main branch
git push --force-with-lease  # push the changes to the PR

Should I make another PR to amend the notes that you made?

The answer to this is nearly always no. A pull request is a request to a project to merge the changes from a branch into another one (the two branches need to be of the same repo but don't need to be on the same fork). Therefore, in order to update a PR you should commit to that branch, then push. You can see that now, the PR includes 7 commits instead of the original 3 because my suggestions have been added as commits to this branch. After the git pull step you will also see those commits on your local branch if you use git log and if needed you can continue to work on the files locally and then commit and push in order to update the PR without needing to open a new one.

@reshamas
Copy link
Contributor

@rowangayleschaefer Let us know if you have any questions on this PR.

@rowan-schaefer
Copy link
Contributor Author

Thank you @OriolAbril @reshamas Didn't see this before. I just tried rebase + push with instructions.

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.

Thanks for the rebase, it was great having the preview to check how things look.

pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/sample_smc.py Outdated Show resolved Hide resolved
pymc/smc/smc.py Outdated Show resolved Hide resolved
pymc/smc/smc.py Outdated Show resolved Hide resolved
pymc/smc/smc.py Outdated Show resolved Hide resolved
pymc/smc/smc.py Outdated Show resolved Hide resolved
@OriolAbril
Copy link
Member

Maybe the deflist inside the quotes looks better for the kwarg descriptions? Preview of both options: https://pymcio--6114.org.readthedocs.build/projects/docs/en/6114/api/generated/pymc.smc.sample_smc.html cc @rowangayleschaefer @ricardoV94 @aloctavodia

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

@rowangayleschaefer this looks great. Just a warning that I might try to merge #6171 first, in which case this PR will probably have to be updated

@reshamas
Copy link
Contributor

reshamas commented Nov 8, 2022

FYI.
#6171 has been merged in.
#6174 has been merged in.

@rowangayleschaefer Would you like to sync and then update the PR?

@michaelosthege michaelosthege added docs SMC Sequential Monte Carlo labels Nov 19, 2022
@michaelosthege michaelosthege added this to the v4.4.0 milestone Nov 19, 2022
Rowan Schaefer and others added 6 commits November 19, 2022 14:02
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
rowan-schaefer and others added 6 commits November 19, 2022 14:02
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
@michaelosthege
Copy link
Member

After rebasing I'll now merge, so all these nice docs fixes get rolled out with the upcoming v4.4.0 🚀

Thanks @rowangayleschaefer and reviewers!

@canyon289 canyon289 merged commit e153121 into pymc-devs:main Nov 19, 2022
@canyon289
Copy link
Member

Thank you Rowan

wrongu pushed a commit to wrongu/pymc that referenced this pull request Dec 1, 2022
* fixed docstrings and a reference

* trailing whitespace

* fixed typo

* Update pymc/smc/sample_smc.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* Update pymc/smc/sample_smc.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* Update pymc/smc/sample_smc.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* Update pymc/smc/sample_smc.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* Update pymc/smc/sample_smc.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* Update pymc/smc/sample_smc.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* Update pymc/smc/smc.py

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>

* Made changes to smc.py and sample_smc.py for pr#6114

* Fix pre-commit

Co-authored-by: Rowan Schaefer <rowanschaefer@Rowans-MacBook-Air.local>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs SMC Sequential Monte Carlo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants