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

minipyro documentation clarification. #3003

Closed
luiarthur opened this issue Jan 11, 2022 · 2 comments
Closed

minipyro documentation clarification. #3003

luiarthur opened this issue Jan 11, 2022 · 2 comments
Labels
documentation help wanted Issues suitable for, and inviting external contributions

Comments

@luiarthur
Copy link
Contributor

I really like that minipyro is in here for those interested in the internals of pyro. Super awesome!

Some of the wording in the module, particularly regarding PYRO_STACK, is confusing to me, though.

For example:

# Handlers earlier in the PYRO_STACK are applied first.

If we run:

trace(replay(model, guide_trace)).get_trace(...)

We end up (somewhere during the execution of the above line) with a PYRO_STACK with the first item referencing trace, and the second item referencing replay. But when we hit apply_stack, because of the reverse order in which the handlers are applied, replay is applied first. So, the comments should read, # Handlers *later* in the PYRO_STACK are applied first., correct?

Similarly, in

def apply_stack(msg):

Shouldn't

When a Messenger sets the "stop" field of a message,
it prevents any Messengers above it on the stack from being applied.

really be:

When a Messenger sets the "stop" field of a message,
it prevents any Messengers below it on the stack from being applied.

And

A Messenger that sets msg["stop"] == True also prevents application
of postprocess_message by Messengers above it on the stack
via the pointer variable from the process_message loop

should really be:

A Messenger that sets msg["stop"] == True also prevents application
of postprocess_message by Messengers below it on the stack
via the pointer variable from the process_message loop

I could be convinced to interpret all this otherwise, but I guess this was my default interpretation. Am I way off-base?

@fritzo
Copy link
Member

fritzo commented Jan 12, 2022

Hi @luiarthur, yes your reading seems to be the correct interpretation. We'd be happy to review a PR fixing or clarifying the comments!

@fritzo fritzo added the help wanted Issues suitable for, and inviting external contributions label Jan 12, 2022
@luiarthur
Copy link
Contributor Author

Cool! I'll start a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation help wanted Issues suitable for, and inviting external contributions
Projects
None yet
Development

No branches or pull requests

2 participants