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

Expose initialization strategy in HMC #2417

Merged
merged 4 commits into from
Apr 16, 2020
Merged

Expose initialization strategy in HMC #2417

merged 4 commits into from
Apr 16, 2020

Conversation

fehiepsi
Copy link
Member

Addresses part of #2415.

There is one issue is: if users want to change keywords of init strategies, they have to use partial, e.g. partial(init_to_median, num_samples=20). I find that it is a bit inconvenient for users. In NumPyro, users can set init_strategy= init_to_median(20) or init_to_uniform(3), init_to_value({"x": 10}), init_to_feasible().

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

Re: usability, you could use the standard poutine trick:

def init_to_uniform(site=None, radius=2):
    if site is None:
        return functools.partial(init_to_uniform, radius=2)
    ...

Then users can pass init_to_uniform(radius=0.5) as their function.

fritzo
fritzo previously approved these changes Apr 16, 2020
Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

This is great!

Do I understand correctly that this allows me to specify a subset of init_params to do so in the constrained space (unlike the initial_params kwarg, which if so we should mark DEPRECATED).

@fehiepsi
Copy link
Member Author

@fritzo initial_params is originally useful when users use potential_fn instead of model.

Re: usability, you could use the standard poutine trick:

Thanks, yeah we should do it!

@fehiepsi
Copy link
Member Author

Do I understand correctly that this allows me to specify a subset of init_params to do so in the constrained space

Yes, you just need to specify a subset. The remaining params use the uniform strategy (default in MCMC).

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

LGTM Thanks for implementing this!

@fritzo fritzo merged commit de0ab28 into pyro-ppl:dev Apr 16, 2020
@neerajprad
Copy link
Member

@fritzo - does this address your concern of initialization code being triggered at various points in the flow despite the user specifying initial params?

@fehiepsi
Copy link
Member Author

@neerajprad This does not. I think we should address it.

@fehiepsi
Copy link
Member Author

I am fixing some typos. I'll address that too.

@neerajprad
Copy link
Member

Thanks for clarifying, @fehiepsi. That might involve some refactoring, look forward to seeing your PR. Let me know if I can help.

@fritzo
Copy link
Member

fritzo commented Apr 16, 2020

does this address your concern of initialization code being triggered

Yes this addresses my concern with duplicated work. The problem was, the model was running 100 random initializations even if I provided a single good one. After This PR, if I provide a single good initialization through init_to_value, no random initializations are sampled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants