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

(Kernel DSL) Revert esc change toplevel, include GlobalRef to handle evaluation ambiguity in modules/submodules. #463

Merged
merged 6 commits into from
May 17, 2022

Conversation

femtomc
Copy link
Contributor

@femtomc femtomc commented Apr 14, 2022

No description provided.

@femtomc femtomc requested a review from ztangent April 14, 2022 14:30
Copy link
Member

@ztangent ztangent left a comment

Choose a reason for hiding this comment

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

Looks great, just suggested a few style changes for greater readability!

src/inference/kernel_dsl.jl Outdated Show resolved Hide resolved
src/inference/kernel_dsl.jl Outdated Show resolved Hide resolved
@femtomc femtomc requested a review from ztangent April 15, 2022 03:10
Copy link
Member

@ztangent ztangent left a comment

Choose a reason for hiding this comment

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

Just have a minor comment about whether to make the GlobalRefs const, since they are global variables now. Fine either way though!

@ztangent
Copy link
Member

Oh, the other thing we should do is add a test case that makes sure the error you ran into earlier (of using a custom proposal) doesn't happen again!

@femtomc
Copy link
Contributor Author

femtomc commented Apr 15, 2022

Re -- I'll address the const issue and think up a test case!

@femtomc femtomc requested a review from ztangent May 16, 2022 13:26
Copy link
Member

@ztangent ztangent left a comment

Choose a reason for hiding this comment

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

Looks great to me! For the test cases, maybe we should also test a kernel that uses a custom proposal, to ensure we don't reintroduce that bug again?

@femtomc
Copy link
Contributor Author

femtomc commented May 16, 2022

@ztangent I don't think the issue was using a custom proposal in MH iirc (?) One of the issues was argument name capture.

Or, do you mean -- capturing the name of the proposal in the kernel at module scope?

I can (and should) easily add the latter.

@ztangent
Copy link
Member

capturing the name of the proposal in the kernel at module scope

Yup, this is what I meant! Because it's the issue that came up when we tried not-escaping everything.

@ztangent ztangent merged commit f9aef08 into master May 17, 2022
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