Skip to content

Refactor get_tau_sigma and support lists of variables #6988

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

Closed
wants to merge 0 commits into from

Conversation

tvwenger
Copy link
Contributor

@tvwenger tvwenger commented Nov 3, 2023

What is this PR about?
This PR refactors the get_tau_sigma logic in order to support lists of Variables and fixes #6987.

N.B. The original code had a positivity check for non-Variable arguments, but not for Variable arguments. Since this PR casts the arguments to Variable, I've removed the positivity check for non-Variable arguments.

Checklist

Major / Breaking Changes

  • get_tau_sigma now always returns a Variable, even when scalars are passed

New features

  • N/A

Bugfixes

Documentation

  • N/A

Maintenance

  • Updated tests

📚 Documentation preview 📚: https://pymc--6988.org.readthedocs.build/en/6988/

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Attention: Patch coverage is 90.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.73%. Comparing base (46f1675) to head (bad3b80).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6988       +/-   ##
===========================================
- Coverage   89.60%   77.73%   -11.88%     
===========================================
  Files         100      100               
  Lines       16888    16880        -8     
===========================================
- Hits        15133    13121     -2012     
- Misses       1755     3759     +2004     
Files Coverage Δ
pymc/distributions/shape_utils.py 59.89% <ø> (-36.05%) ⬇️
pymc/model/core.py 72.15% <ø> (-19.18%) ⬇️
pymc/model_graph.py 14.57% <ø> (-62.82%) ⬇️
pymc/pytensorf.py 73.61% <ø> (-17.42%) ⬇️
pymc/distributions/continuous.py 64.64% <90.00%> (-32.90%) ⬇️

... and 40 files with indirect coverage changes

@ricardoV94
Copy link
Member

@tvwenger would you like to update this PR? Would only need to add some regression tests for the original issue

@tvwenger
Copy link
Contributor Author

tvwenger commented Mar 1, 2024

@ricardoV94 Thanks for the reminder and sorry for the delay. I added some regression tests, so this should be good to go once the checks are finished.

@tvwenger
Copy link
Contributor Author

tvwenger commented Mar 2, 2024

@ricardoV94 This PR is ready for review

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.

This looks good, just two nitpicks, one of which is not related to your changes. Let me know if you want me to merge or if you want to address them.

@tvwenger
Copy link
Contributor Author

tvwenger commented Mar 4, 2024

@ricardoV94 Ready for review when checks are done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: get_tau_sigma does not support list of variables
2 participants