pymc icon indicating copy to clipboard operation
pymc copied to clipboard

Update docstrings for sample_smc and smc.py

Open rowan-schaefer opened this issue 1 year ago • 7 comments

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

  • ...

rowan-schaefer avatar Sep 09 '22 17:09 rowan-schaefer

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% <ø> (ø)

codecov[bot] avatar Sep 09 '22 17:09 codecov[bot]

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

cluhmann avatar Sep 11 '22 13:09 cluhmann

hopefully the build error gets fixed soon!

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

OriolAbril avatar Sep 11 '22 13:09 OriolAbril

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

rowan-schaefer avatar Sep 12 '22 16:09 rowan-schaefer

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.

reshamas avatar Sep 13 '22 19:09 reshamas

Done!

rowan-schaefer avatar Sep 13 '22 21:09 rowan-schaefer

@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.

OriolAbril avatar Sep 14 '22 15:09 OriolAbril

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

reshamas avatar Oct 13 '22 12:10 reshamas

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

rowan-schaefer avatar Oct 13 '22 14:10 rowan-schaefer

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

OriolAbril avatar Oct 14 '22 09:10 OriolAbril

@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

ricardoV94 avatar Oct 28 '22 08:10 ricardoV94

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

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

reshamas avatar Nov 08 '22 19:11 reshamas

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!

michaelosthege avatar Nov 19 '22 17:11 michaelosthege

Thank you Rowan

canyon289 avatar Nov 19 '22 17:11 canyon289