-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fix merge_metadata
for cases where the dimension of the variable changes
#781
Conversation
Pull Request Test Coverage Report for Build 12885489364Details
💛 - Coveralls |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #781 +/- ##
==========================================
- Coverage 86.25% 86.16% -0.10%
==========================================
Files 36 36
Lines 4330 4301 -29
==========================================
- Hits 3735 3706 -29
Misses 595 595 ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 12907177100Details
💛 - 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.
Lgtm 👍
Thanks @penelopeysm. I'll give this a few hours to see if anyone who knows the old code wants to rush in shouting "STOP THE MERGE". |
Weird. The only difference between the runs is that ADTypes was upgraded from 1.11.0 to 1.12.0, which was a change to AutoFiniteDiff that isn't used anywhere in our code. |
OK, I found that KernelAbstractions.jl also got bumped from 0.9.31 to 0.9.32, and I think it's this JuliaGPU/KernelAbstractions.jl#551 that broke our code. It seems that for now a suitable solution is to pin the version of KernelAbstractions (we should do this in the toplevel Project.toml, not just the test one, because with the newer version JET will infer an untyped varinfo when it's not really needed). I tested locally and can confirm that this fixes the failing test. Longer term we should make a smaller repro and post an upstream issue. |
Co-authored-by: Penelope Yong <[email protected]>
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.
ell gee tee em
The diff here is actually much simpler than it looks. The only real change in
src/varinfo.jl
is removing the line@assert length(vals_left) == length(vals_right)
, in theif vn in vns_left && vn in vns_right
path. It's just that once you remove that line, you can simplify the surrounding code, because two of the if-paths become exactly identical and the third one nearly so.I don't see any reason why we couldn't, when merging two metadata objects, merge just the same in the case where the dimension of some variable is different on the left and the right. I asked @torfjelde and he couldn't think of a reason either, but if someone else knows why the
@assert
was there in the first place, please say so.This change would resolve TuringLang/Turing.jl#2472.