-
Notifications
You must be signed in to change notification settings - Fork 8
Make prefix
more type-stable
#121
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 90.93% 91.02% +0.09%
==========================================
Files 2 2
Lines 298 301 +3
==========================================
+ Hits 271 274 +3
Misses 27 27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 14550376651Details
💛 - Coveralls |
It's probably always a good idea to make the compiler's life easier where possible. |
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.
it looks fine to me and I don't see a downside for this over the original implementation.
Regarding fixing type stability errors, I has couple of thoughts, none of them are too serious:
-
It appears to me that the compiler is smart enough to figure out that getoptic(vn) returns the second type parameter of vn, and thus when it looks at the line if getoptic(vn) == identity it can statically determine whether this is true or false, and infer the return type appropriately.
is it possible to test the behavior of the compiler in some way? (I am thinking inspecting
IRCode
) -
(I don't have a good enough knowledge of the Julia compiler.)
Topic_prefix
is nested, right? Do we have an examples of reasonably deeply nested VarName? So that we can know that the compiler doesn't give up too easily?
While trying to hunt down type instabilities in TuringLang/DynamicPPL.jl#892 this was one of the things that I first looked at. I thought that the changes in this PR would make a difference. However, simply testing
@inferred AbstractPPL.prefix(vn1, vn2)
with various combinations ofvn1
andvn2
, I can't actually see any difference on Julia 1.11.5, and it didn't help with any upstream type instabilities.It appears to me that the compiler is smart enough to figure out that
getoptic(vn)
returns the second type parameter ofvn
, and thus when it looks at the lineif getoptic(vn) == identity
it can statically determine whether this is true or false, and infer the return type appropriately.I don't know if it's risky to rely on this behaviour, hence why I'm keeping this PR open.