Skip to content

Fix setting initvals on ZeroSumTransform rv #7773

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

velochy
Copy link
Contributor

@velochy velochy commented May 2, 2025

Description

Fix setting initvals on ZeroSumTransform'ed rv

Related Issue

Checklist

Type of change

  • Bug fix

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

@github-actions github-actions bot added the bug label May 2, 2025
@velochy
Copy link
Contributor Author

velochy commented May 2, 2025

Is a new test required for this?

I can easily add the one in the issue, but it seems a bit of an overkill, considering it will sample a model.

Copy link

codecov bot commented May 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.84%. Comparing base (bd28ee9) to head (8adacfa).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7773   +/-   ##
=======================================
  Coverage   92.83%   92.84%           
=======================================
  Files         107      107           
  Lines       18354    18378   +24     
=======================================
+ Hits        17039    17063   +24     
  Misses       1315     1315           
Files with missing lines Coverage Δ
pymc/distributions/transforms.py 98.59% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

It's a staticmethod, so a unit test could test just that.

The extend_axis_rev below it has the same bug.

@velochy
Copy link
Contributor Author

velochy commented May 2, 2025

It's a staticmethod, so a unit test could test just that.

The extend_axis_rev below it has the same bug.

Fixed the one below and added a few basic tests.

Turns out, this transform was only tested through ZeroSumNormal before, so had to add a new segment for it in transforms

@@ -312,7 +312,7 @@ def extend_axis(array, axis):
def extend_axis_rev(array, axis):
normalized_axis = normalize_axis_tuple(axis, array.ndim)[0]

n = array.shape[normalized_axis].astype("floatX")
n = pt.cast(array.shape[normalized_axis], "floatX")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead you can call pt.as_tensor(array)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering, is it better than pt.cast for some reason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I was just thinking out loud

@ricardoV94
Copy link
Member

One test is failing now due to precision issues?

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

Successfully merging this pull request may close these issues.

BUG: ZeroSumTransform fails with initvalues
3 participants