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

Patch constraints over anonymous variables bug #150

Merged
merged 3 commits into from
Jan 15, 2024

Conversation

wouterwln
Copy link
Member

This PR adds the discussed tests and fixes a bug where if a constraint is applied to a node that has an anonymous variable that has two linked variables we didn't throw an error (and also if multiple variables were anonymous we'd throw). There's two things I'd like you to check @bvdmitri :

  • There's a test failing in the anonymous variables code. According to me, it fails correctly and this test was wrong, but I'd rather that you double check.
  • Constraints resolved and applied over deterministic nodes do nothing. We only apply constraints to stochastic nodes.

@@ -118,7 +124,7 @@ end
y[1] ~ Normal(0, 1)
for i in 2:10
y[i] ~ Normal(0, 1)
x[i] ~ Normal(y[i - 1] + y[i], 1)
y[i] ~ Normal(y[i - 1] + y[i], 1)
Copy link
Member

Choose a reason for hiding this comment

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

This is a strange change? and the strange model, what is actually happening here?

@bvdmitri
Copy link
Member

bvdmitri commented Jan 15, 2024

this test was wrong

The test is correct and the change with the length does not take into account that some linked variables maybe constants. So the length check is not applicable here.

@bvdmitri
Copy link
Member

I reviewed the change, reverted the length check and fixed the test. Please double check :)

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a6ea9ed) 92.44% compared to head (4d21e2c) 92.38%.

Additional details and impacted files
@@              Coverage Diff              @@
##           dev-4.0.0     #150      +/-   ##
=============================================
- Coverage      92.44%   92.38%   -0.06%     
=============================================
  Files              9        8       -1     
  Lines           1417     1406      -11     
=============================================
- Hits            1310     1299      -11     
  Misses           107      107              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wouterwln
Copy link
Member Author

AFAIK this is correct, I think we can merge if you also agree

@bvdmitri bvdmitri merged commit 14352e6 into dev-4.0.0 Jan 15, 2024
4 of 5 checks passed
@wouterwln wouterwln deleted the mixture-constraints-bug branch February 16, 2024 13:44
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.

2 participants