Skip to content

Draft: Fix the bug: PrecisionMvNormalRV missing factor of 1/2 for log determinant in logp function #7811

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

olp-cs
Copy link

@olp-cs olp-cs commented Jun 7, 2025

Description

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify)

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

Copy link

welcome bot commented Jun 7, 2025

Thank You Banner]
💖 Thanks for opening this pull request! 💖 The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.

@github-actions github-actions bot added bug hackathon Suitable for hackathon labels Jun 7, 2025
@ricardoV94
Copy link
Member

@olp-cs there's a PR open for this, and I left a comment there about adding a regression test. The same applies here.

@asifzubair
Copy link
Contributor

Hi @olp-cs , are you still working on this PR ? If not, I'd be happy to pick it up. Please let me know. 🙏

@olp-cs
Copy link
Author

olp-cs commented Aug 8, 2025

@asifzubair I've recently came back to it. However, if you don't see any updates by 11 August, feel free to take it over. Thank you!

@olp-cs
Copy link
Author

olp-cs commented Aug 11, 2025

@asifzubair

If not, I'd be happy to pick it up.

Please do; I would really appreciate it. I've reviewed test_multivariate.py but I’m not familiar enough with this part to add tests without asking a lot of questions (and taking up your time), so I’ll gladly let you handle this one and follow your commits to learn. For context, the following test was proposed in a similar PR:

If tau is a unit diagonal precision matrix the logp should match the save with the covariance logp

@asifzubair
Copy link
Contributor

Sorry, @olp-cs , I should've followed up with you. Thank you for letting me tak a look at this. I left a comment on the issue. I'm not sure this fix is required. Please let me know if you disagree with my comment. I was also wondering if you ran tests after making your change and noticed that this test was failing. Thanks, again! 🙏

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

Successfully merging this pull request may close these issues.

BUG: PrecisionMvNormalRV missing factor of 1/2 for log determinant in logp function
3 participants