-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Reverse order of prefixing & add changelog #792
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-0.35 #792 +/- ##
================================================
- Coverage 86.17% 86.17% -0.01%
================================================
Files 36 36
Lines 4305 4304 -1
================================================
- Hits 3710 3709 -1
Misses 595 595 ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 13012939391Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy with this. Note also that this restores the old behavior of @submodel
:
module MWE
using DynamicPPL, Distributions
@model function subsubmodel()
x ~ Normal()
end
@model function submodel()
@submodel prefix="c" x = subsubmodel()
return x
end
@model function parentmodel()
@submodel prefix="a" x1 = submodel()
@submodel prefix="b" x2 = submodel()
end
@show keys(VarInfo(parentmodel()))
end
keys(VarInfo(parentmodel())) = AbstractPPL.VarName{sym, typeof(identity)} where sym[a.c.x, b.c.x]
Hold on, I didn't realise that. 😅 In that case should it just be a patch release? |
You could argue that. Depends on whether we consider the new behavior of |
Which one is more likely? That someone hasn't adjusted to the new prefixing order of Maybe the latter? |
Aaah, yeah. I guess the other interpretation is that to_submodel's behaviour is defined to be the same as macro submodel, but anyway, let's stick to minor release then, since (a) it's less work; and (b) there's not much harm in releasing a patch as a minor (as opposed to the other way round). |
Maybe add a mention in HISTORY.md though, that this is how |
Done! |
Previously, the variable names would be
c.a.x
andc.b.x
. This PR switches the order so that it'sa.c.x
andb.c.x
, which is more intuitive.This PR also adds a changelog. I copied the release notes which I wrote for the last few releases.
Targetting the 0.35 branch because this is a breaking change in behaviour.
Closes #786