Proper support for distributions with embedded support #462
Conversation
methods for linking and invlinking
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…rt' into torfjelde/support-embedded-support
to be more descriptive
…rt' into torfjelde/support-embedded-support
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
I've solved the issue with AMH locally (it wasn't copying the return parameters, so they were all fixed to the same value) but I'm very confused as to why this hasn't been an issue before, i.e. why is this PR causing it to show up only now. Currently digging into this. |
…gLang/DynamicPPL.jl into torfjelde/support-embedded-support
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Aight, I think this is good to go! Now there are quite a few PRs in the pipeline that will fix sampling for some distributions once we have this PR through, e.g. TuringLang/Bijectors.jl#246 and TuringLang/Bijectors.jl#263. Hence it would be nice to get this merged. My only worry with this PR is that it introduces some performance degradation, but by looking at the runtimes of the tests, it doesn't seem particularly significant + correctness of the program takes precedence over performance so it seems worth it (and then we can fix potential perf issues later on). |
|
I'll get this merged once TuringLang/Turing.jl#2001 has gone through so we can also run the integration tests (currently Turing.jl isn't comatible with 0.23, so I can't run the integration tests). |
|
Why was this merged? Was it merged before the Turing version was released? |
|
It’s merged after the new Turing release |
|
But then how did it make it's way through the merge queue when the Turing tests are failing on master? |
|
I guess it's because the Turing test status is not required anymore (since as previously noticed otherwise it's not possible to add the PR to the merge queue). Ultimately, I guess the problem is as discussed in e.g. https://github.com/orgs/community/discussions/46757#discussioncomment-5786544 and https://github.com/orgs/community/discussions/46757#discussioncomment-6035779: Github does not distinguish between status checks that are required to add a PR to the queue and status checks that are required to merge a PR in the queue into the desired branch. It seems currently Github merge queue is not a proper replacement for bors 😞 |
This reverts commit 7b01d25.
|
Oooh true I forgot we were also discussing how to do this a few weeks ago. |
Attempt at addressing #461.
I think the approach here is somewhat correct, but it's currently very dirty because of the intricacies of
VarInfoimplementation. This can be cleaned up, but will take some effort. Until I've done this, I leave this as a draft PR.We also most certainly need to do some benchmarking before merging this as it could lead to some additional overhead.
NOTE: This is based on #457