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

change constructor of OrnsteinUhlenbeckDiffusion #31

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bicycle1885
Copy link
Collaborator

I'd like to propose changing the parametrization of the constructor of OrnsteinUhlenbeckDiffusion. The rationale behind this is that we typically want to tune the speed of convergence by changing the reversion parameter while keeping the distribution at equilibrium unchanged. Also, I think we won't tune the mean parameter in most cases because it is not very important in diffusion models. I also added a short docstring to the constructor.

@murrellb
Copy link
Member

murrellb commented Mar 8, 2023

I also usually work with a mean,reversion,eq_std parameterization, so I'm happy with this direction. The optional arguments should allow the equilibrium std to be input directly. Maybe we call that eqσ to avoid confusing it with the volatility? So I should be able to call OrnsteinUhlenbeckDiffusion(1.0, μ = 5.0, eqσ = 10.0) where the defaults would be μ = 0, eqσ = 1 if not specified.

@bicycle1885
Copy link
Collaborator Author

I'm wondering how often we will change the parameter σ in practice. I think most users won't tune that parameter; if they do, they must know what they do. Introducing a new parameter like eqσ might be rather confusing for those who are already familiar with the volatility parameter and want to tune it for some reason.

@murrellb
Copy link
Member

murrellb commented Mar 8, 2023

Playing with toy cases, the equilibrium std can make a practical difference for how easily the model learns. It is one of the things I tune. And anyone that knows what they're doing will understand the meaning of "eq", and can set the parameters directly if they like!

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