-
-
Notifications
You must be signed in to change notification settings - Fork 985
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
add substitute handler #3125
add substitute handler #3125
Conversation
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.
Looks great and I especially like the name checking for usability!
- Could you explain the need for the _data_cache? Would this PR break if that were missing, or is it merely for speed?
- Could you add some unit tests in tests/poutine/test_poutines.py, perhaps forking
ConditionHandlerTests
? - Could you add an entry in docs/source/pyro.poutine.rst (inserting in alphabetical order please 😉)
- Looks like you'll need to run
make format
to fix coding style (see CONTRIBUTING.md), that's why the lint stage is failing. You can test this locally viamake lint
.
Hi @fritzo, My guess is: It's probably ok to drop TBH, I have not looked into Pyro's code too much, and was largely mimicing the lift handler. For the test case, I'm not sure how |
@fritzo I added some tests, but I don't know how to run the test locally... Sorry
produces:
|
I'm not sure why your local |
Hi @thisiscam, sorry this seems to have fallen off the radar and CI for some reason didn't trigger. Could you try merging in the dev branch and re-pushing? |
merge in the dev (since CI didn't trigger last time)
@fritzo Hi, sorry about the delay. I just merged |
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.
Looks great, thanks for working through the CI issues.
Attempt at #3124, largely mimics the
lift
handler