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

Add parameter to allow disabling noise term scaling for BAOAB #520

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

jebej
Copy link
Contributor

@jebej jebej commented Jan 6, 2023

As discussed in #519, I want to experiment with removing the gamma dependence attached to the noise term for the BAOAB algorithm.

The main issue is that if you do want the gamma dependence, you also need a dependence on dt (coming from the c1 variable in the original code). While this dt dependence could be user-coded in the g noise function, it's not an ideal interface.

Suggestions welcome.

EDIT: instead of removing the scaling wholesale, there is now a scale_noise::Bool argument to BAOAB.

@rmsrosa
Copy link
Contributor

rmsrosa commented Jan 7, 2023

I am not familiar with the method, but browsing the discussions, what about, instead of removing c2, adding another keyword argument to BAOAB() with the option of changing the default behavior?

@ChrisRackauckas
Copy link
Member

I just suggested that same thing in another way. I agree it should just have two kwargs.

@jebej
Copy link
Contributor Author

jebej commented Jan 9, 2023

If we want another keyword argument, the only thing I can think of is something like scale_noise=true, which keeps the current behavior, and when false just sets the c2 variable to 1. Anything else can be handled in the user function.

@ChrisRackauckas
Copy link
Member

Or if it's another float, you just make the default c2 = gamma.

@ChrisRackauckas
Copy link
Member

Either way is fine I think, but I think the main point is that it needs two args not one.

@jebej
Copy link
Contributor Author

jebej commented Jan 9, 2023

If we want the original behavior from the paper we need dt:

c1 = exp(-alg.gamma*dt)
c2 = sqrt(1 - c1^2)

@ChrisRackauckas
Copy link
Member

It's not already scaling by dt?

@jebej
Copy link
Contributor Author

jebej commented Jan 9, 2023

From https://doi.org/10.1093/amrx/abs010 (c2 and c3 were switched)
image

@ChrisRackauckas ChrisRackauckas merged commit cb32ff2 into SciML:master Jan 9, 2023
@jebej jebej changed the title remove gamma param from noise term for BAOAB alg Add parameter to allow disabling noise term scaling for BAOAB Jan 9, 2023
@jebej jebej deleted the baoab-mod branch January 9, 2023 22:11
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.

3 participants