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

Updated docstring for Potential function #6559

Merged
merged 3 commits into from
Mar 5, 2023

Conversation

Dhruvanshu-Joshi
Copy link
Member

What is this PR about?
This PR is a continuation of the PR 6489 in order to solve the merge conflicts.
In this PR , I have provided a solution to close the issue 6399.
The docstring for the Potential function is rather limited. I have made efforts to include some code examples to better understand this. I also focused on the nature of the arguments that are passed to this function and have included an example for the same.

Checklist

Documentation

  • Add code example and a short explanation of the code example to Potential docstrings.
  • In the first example, we define a constraint such that the probability density that the model produces takes into consideration that x must be greater than or equal to 0.
  • In the second example, we emphasis on the usage of soft constraints.
  • The third example focuses on how the Potential function is used to obtain an arbitrary prior.
  • In the final example, the soft_sum_constraint potential encourages x and y to have a small sum, effectively adding a soft constraint on the relationship between the two variables.

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #6559 (2e2dcf3) into main (2f945bb) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6559   +/-   ##
=======================================
  Coverage   92.01%   92.01%           
=======================================
  Files          91       91           
  Lines       15114    15114           
=======================================
  Hits        13907    13907           
  Misses       1207     1207           
Impacted Files Coverage Δ
pymc/model.py 89.52% <ø> (ø)

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.

The explanations are good!

I just nitpicked about some small code/docstring style things. You can add line breaks - at least to the level of the "1 sentence per line" rule of thumb.

pymc/model.py Outdated Show resolved Hide resolved
pymc/model.py Outdated Show resolved Hide resolved
pymc/model.py Outdated Show resolved Hide resolved
pymc/model.py Outdated Show resolved Hide resolved
pymc/model.py Outdated Show resolved Hide resolved
pymc/model.py Outdated Show resolved Hide resolved
@Dhruvanshu-Joshi
Copy link
Member Author

Hey @michaelosthege . Thanks for the review. I have included all the suggestions in the new commit.

pymc/model.py Outdated Show resolved Hide resolved
@michaelosthege michaelosthege merged commit 253aaa8 into pymc-devs:main Mar 5, 2023
@chriswmann
Copy link

Bad timing since this was just merged but I think there are a couple of typos in the merged commit. E.g. line 2080 says "We pass -pm.math.log(pm.math.switch(constraint, 1, 0)) as second argument" but then in the code example on line 2089, the actual argument is pm.math.log(pm.math.switch(constraint, 1, 0.0)) (i.e. the argument is not negated). Same again for the soft constraint on lines 2091 and 2103. I'm happy to raise an issue and submit a PR if it is agreed these are corrections that should be made.

@ricardoV94
Copy link
Member

@chriswmann thanks that would be appreciated

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