Improve `join_nonshared_inputs` documentation #6216

wd60622 · 2022-10-14T01:27:33Z

What is this PR about?
This PR closes issue 6105 and increase the documentation of join_nonshared_inputs function in pymc/aesaraf.py

Checklist

Explain important implementation details 👆
Make sure that the pre-commit linting/style checks pass.
Link relevant issues (preferably in nice commit messages)
Are the changes covered by tests and docstrings?
Fill out the short summary sections 👇

Major / Breaking Changes

...

Bugfixes / New features

...

Docs / Maintenance

Adding type hints for join_nonshared_inputs and improving docstrings

…metropolis alg

codecov · 2022-10-14T01:50:27Z

Codecov Report

Merging #6216 (8360f86) into main (ea721e4) will decrease coverage by 2.85%.
The diff coverage is 81.81%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6216      +/-   ##
==========================================
- Coverage   94.14%   91.29%   -2.86%     
==========================================
  Files         100      100              
  Lines       21239    21240       +1     
==========================================
- Hits        19996    19391     -605     
- Misses       1243     1849     +606

Impacted Files	Coverage Δ
pymc/aesaraf.py	`90.15% <76.47%> (-3.39%)`	⬇️
pymc/smc/kernels.py	`97.37% <100.00%> (ø)`
pymc/step_methods/metropolis.py	`54.75% <100.00%> (-28.66%)`	⬇️
pymc/tests/step_methods/test_slicer.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/step_methods/hmc/test_nuts.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/tests/step_methods/test_metropolis.py	`0.00% <0.00%> (-100.00%)`	⬇️
pymc/data.py	`47.96% <0.00%> (-32.12%)`	⬇️
pymc/step_methods/arraystep.py	`89.36% <0.00%> (-4.97%)`	⬇️
... and 3 more

wd60622 · 2022-10-14T03:47:09Z

still WIP. Would like to add more to the docstring. The current example is similar to that of the delta_logp in the metropolis.py

pymc/aesaraf.py

wd60622 · 2022-10-19T19:31:51Z

Here is the preview

ricardoV94 · 2022-10-19T19:35:15Z

Thanks for opening the PR, I will try to review tomorrow

wd60622 · 2022-10-19T19:45:48Z

Thanks for opening the PR, I will try to review tomorrow

thanks. no rush

pymc/aesaraf.py

ricardoV94

Great start! I left some suggestions to be more precise, and more succinct / readable examples. Let me know what you think.

While reviewing I also realized we could come up with much better argument names for the function (and internal variables) instead of xs, vars, and inarray, xs_special. Probably outputs, inputs, joined_inputs, new_outputs, would be more readable?

pymc/aesaraf.py

wd60622 · 2022-10-20T18:36:51Z

Thanks for all the suggestions. All very good and am working through the edits

pymc/aesaraf.py

wd60622 · 2022-10-25T11:42:48Z

Docs seem to be looking good! https://pymcio--6216.org.readthedocs.build/projects/docs/en/6216/api/generated/pymc.join_nonshared_inputs.html#pymc.join_nonshared_inputs

ricardoV94

This looks amazing! I left one type-hint suggestion and a question that I am not sure about

pymc/aesaraf.py

wd60622 · 2022-10-26T22:55:24Z

everything should be addressed

The latest: https://pymcio--6216.org.readthedocs.build/projects/docs/en/6216/api/generated/pymc.join_nonshared_inputs.html#pymc.join_nonshared_inputs

EDIT: I forgot to change the one dict mapping lol

ricardoV94 · 2022-10-27T06:34:40Z

The failing test seems unrelated and I think I already saw it failing before. Do you mind opening an issue to track it?

michaelosthege · 2022-10-30T10:03:28Z

Looks like the recent renaming of the SMC files introduced conflicts..

I will give it a try at rebasing

…metropolis alg

michaelosthege · 2022-10-30T13:12:04Z

@wd60622 I was confused why GitHub doesn't show your profile picture (and link) on the commits after I rebased them.
I think this is because the gmail address from the commit information is maybe not linked to your GitHub profile?

You can set it up under https://github.com/settings/emails and then GitHub should show your avatar on the commit messages again.

OriolAbril · 2022-10-30T14:05:37Z

pymc/aesaraf.py

-    point: a sample point
-    xs: list of Aesara tensors
-    vars: list of variables to join
+    point : Point


I am not sure point is the right type. It does not match the type signature and it is not a "metatype" meaning a subset of dicts that match these properties but a different amd specific class. Does anybody know?

Yeah. I'm not sure either. I was going off Point being in reference link and the fact that Point function would construct the same type as the alternative.
i.e. Dict[str, np.ndarray]

let me know which fits best with the package

Dict[str, np.ndarray] is the correct type here, but I'd say Point is also fine because users/devs probably understand what is meant in this context.

With the exact type hints in the signature, I treat the hints in the docstring as the "human readable" version, and often times I leave them away to not duplicate the information.

What is our policy, @OriolAbril ?

Then this is what should be used (with the correft numpydoc formatting) instead of point which is therefore an invalid type. This function is on the website and it needs to have the type info properly documented in the docstring having users in mind

Okay. So I am hearing to switch back to the dict of {str : array_like}. Is that correct?

Why is Point referenced in the docs/.../conf.py
Is there a purpose or usecase?

wd60622 · 2022-10-30T16:17:54Z

@wd60622 I was confused why GitHub doesn't show your profile picture (and link) on the commits after I rebased them. I think this is because the gmail address from the commit information is maybe not linked to your GitHub profile?

All fixed. I had my account email switch recently. Thanks for the tip. Looks more legit now 😄

wd60622 · 2022-10-30T16:20:22Z

Not sure why the doc build failed. I haven't seen that error before

OriolAbril · 2022-10-30T20:48:03Z

The readthedocs error is a conda error don't have a clue as to why this is happening though. Hopefully it is a fluke

wd60622 · 2022-11-01T16:57:19Z

Any more items for this PR?

michaelosthege · 2022-11-01T17:07:52Z

Any more items for this PR?

IIRC that one Point should become Dict[str, np.ndarray]

wd60622 · 2022-11-01T17:29:25Z

Any more items for this PR?

IIRC that one Point should become Dict[str, np.ndarray]

Gotcha. Never got confirmation there.

So the type hint is Dict[str, np.ndarray]. However, the docs parameter description is Point. I will change them to Dict[str, np.ndarray] and "dict of {str : array_like}"

…60622/pymc into aesaraf_join_shared_input_doc

wd60622 · 2022-11-02T12:12:31Z

The code coverage seems unrelated to my changes. and FYI I'm unable to merge bc access

michaelosthege · 2022-11-02T14:56:49Z

New git conflicts 😕
I think they might come from some of the 22 commits in the history, because the final diff doesn't change files that would explain the conflict (as far as I can tell).

Squashing the commits from the branch might already help to resolve the conflicts, and should probably be done before rebasing it

ricardoV94 · 2022-11-02T16:35:26Z

Thanks @wd60622!

wd60622 · 2022-11-02T16:40:54Z

Thanks @wd60622!

Thank you for the help and feedback!

adding type hints, using keyword args, and start of example based on …

ca8862a

…metropolis alg

adding second example and some more to the docs

d70bb6f

wd60622 commented Oct 19, 2022

View reviewed changes

pymc/aesaraf.py Show resolved Hide resolved

wd60622 marked this pull request as ready for review October 19, 2022 18:52

ricardoV94 self-assigned this Oct 19, 2022

wd60622 commented Oct 19, 2022

View reviewed changes

pymc/aesaraf.py Outdated Show resolved Hide resolved

ricardoV94 requested changes Oct 20, 2022

View reviewed changes

ricardoV94 changed the title ~~join_nonshared_inputs documentation~~ Improve join_nonshared_inputs documentation Oct 20, 2022

wd60622 commented Oct 20, 2022

View reviewed changes

pymc/aesaraf.py Outdated Show resolved Hide resolved

add the suggestions from PR

564ab09

wd60622 force-pushed the aesaraf_join_shared_input_doc branch from f9abe5c to 564ab09 Compare October 20, 2022 19:41

wd60622 and others added 3 commits October 20, 2022 15:41

Merge branch 'pymc-devs:main' into aesaraf_join_shared_input_doc

ada261a

first example did not render

a526a03

fix typo

f9a165a

ricardoV94 reviewed Oct 25, 2022

View reviewed changes

pymc/aesaraf.py Outdated Show resolved Hide resolved

pymc/aesaraf.py Outdated Show resolved Hide resolved

ricardoV94 added the docs label Oct 25, 2022

OriolAbril reviewed Oct 25, 2022

View reviewed changes

pymc/aesaraf.py Outdated Show resolved Hide resolved

wd60622 and others added 2 commits October 26, 2022 18:50

switch to numpy docstyle. comment in example. fix return

c4be323

Merge branch 'pymc-devs:main' into aesaraf_join_shared_input_doc

117b929

forgotten dict

b3ca27d

ricardoV94 approved these changes Oct 27, 2022

View reviewed changes

wd60622 added 8 commits October 30, 2022 11:43

adding type hints, using keyword args, and start of example based on …

2923927

…metropolis alg

adding second example and some more to the docs

3a8b3fb

add the suggestions from PR

70601f5

first example did not render

acc4f6d

fix typo

fb0a141

switch to numpy docstyle. comment in example. fix return

4f9d687

forgotten dict

a1dc3c4

numpy docstring standards

7065982

michaelosthege force-pushed the aesaraf_join_shared_input_doc branch from fe95d58 to 7065982 Compare October 30, 2022 10:44

michaelosthege approved these changes Oct 30, 2022

View reviewed changes

OriolAbril reviewed Oct 30, 2022

View reviewed changes

wd60622 and others added 3 commits November 1, 2022 18:08

Merge branch 'aesaraf_join_shared_input_doc' of https://github.com/wd…

f37dbaa

…60622/pymc into aesaraf_join_shared_input_doc

switching away from Point

a50be1f

Merge branch 'pymc-devs:main' into aesaraf_join_shared_input_doc

8360f86

michaelosthege approved these changes Nov 1, 2022

View reviewed changes

ricardoV94 requested a review from OriolAbril November 2, 2022 12:20

OriolAbril approved these changes Nov 2, 2022

View reviewed changes

ricardoV94 merged commit b0d1066 into pymc-devs:main Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `join_nonshared_inputs` documentation #6216

Improve `join_nonshared_inputs` documentation #6216

wd60622 commented Oct 14, 2022 •

edited by ricardoV94

codecov bot commented Oct 14, 2022 •

edited

wd60622 commented Oct 14, 2022

wd60622 commented Oct 19, 2022

ricardoV94 commented Oct 19, 2022

wd60622 commented Oct 19, 2022

ricardoV94 left a comment •

edited

wd60622 commented Oct 20, 2022

wd60622 commented Oct 25, 2022

ricardoV94 left a comment

wd60622 commented Oct 26, 2022 •

edited

ricardoV94 commented Oct 27, 2022

michaelosthege commented Oct 30, 2022

michaelosthege commented Oct 30, 2022

OriolAbril Oct 30, 2022

wd60622 Oct 30, 2022

michaelosthege Oct 30, 2022

OriolAbril Oct 30, 2022

wd60622 Oct 30, 2022

wd60622 Oct 30, 2022

wd60622 commented Oct 30, 2022

wd60622 commented Oct 30, 2022

OriolAbril commented Oct 30, 2022

wd60622 commented Nov 1, 2022

michaelosthege commented Nov 1, 2022

wd60622 commented Nov 1, 2022 •

edited

wd60622 commented Nov 2, 2022

michaelosthege commented Nov 2, 2022

ricardoV94 commented Nov 2, 2022

wd60622 commented Nov 2, 2022

Improve join_nonshared_inputs documentation #6216

Improve join_nonshared_inputs documentation #6216

Conversation

wd60622 commented Oct 14, 2022 • edited by ricardoV94

Major / Breaking Changes

Bugfixes / New features

Docs / Maintenance

codecov bot commented Oct 14, 2022 • edited

Codecov Report

wd60622 commented Oct 14, 2022

wd60622 commented Oct 19, 2022

ricardoV94 commented Oct 19, 2022

wd60622 commented Oct 19, 2022

ricardoV94 left a comment • edited

Choose a reason for hiding this comment

wd60622 commented Oct 20, 2022

wd60622 commented Oct 25, 2022

ricardoV94 left a comment

Choose a reason for hiding this comment

wd60622 commented Oct 26, 2022 • edited

ricardoV94 commented Oct 27, 2022

michaelosthege commented Oct 30, 2022

michaelosthege commented Oct 30, 2022

OriolAbril Oct 30, 2022

Choose a reason for hiding this comment

wd60622 Oct 30, 2022

Choose a reason for hiding this comment

michaelosthege Oct 30, 2022

Choose a reason for hiding this comment

OriolAbril Oct 30, 2022

Choose a reason for hiding this comment

wd60622 Oct 30, 2022

Choose a reason for hiding this comment

wd60622 Oct 30, 2022

Choose a reason for hiding this comment

wd60622 commented Oct 30, 2022

wd60622 commented Oct 30, 2022

OriolAbril commented Oct 30, 2022

wd60622 commented Nov 1, 2022

michaelosthege commented Nov 1, 2022

wd60622 commented Nov 1, 2022 • edited

wd60622 commented Nov 2, 2022

michaelosthege commented Nov 2, 2022

ricardoV94 commented Nov 2, 2022

wd60622 commented Nov 2, 2022

Improve `join_nonshared_inputs` documentation #6216

Improve `join_nonshared_inputs` documentation #6216

wd60622 commented Oct 14, 2022 •

edited by ricardoV94

codecov bot commented Oct 14, 2022 •

edited

ricardoV94 left a comment •

edited

wd60622 commented Oct 26, 2022 •

edited

wd60622 commented Nov 1, 2022 •

edited