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

Surrogates model breaks downstream #63

Open
vikram-s-narayan opened this issue Dec 6, 2022 · 4 comments
Open

Surrogates model breaks downstream #63

vikram-s-narayan opened this issue Dec 6, 2022 · 4 comments

Comments

@vikram-s-narayan
Copy link

vikram-s-narayan commented Dec 6, 2022

It looks like moving from QuasiMonteCarlo@0.2.16 to QuasiMonteCarlo@0.2.19 breaks a surrogate model called GEKPLS downstream in Surrogates.jl.

This gist may help pinpoint the issue.

@vikram-s-narayan
Copy link
Author

Additional info:

It looks like GoldenSample is generating a different set of points with version 0.2.19.

QuasiMonteCarlo@0.2.19 includes the lower bound as the first sample when we use GoldenSample(). This was not the case with QuasiMonteCarlo@0.2.16 which maintained an offset. For example, if we do:


using QuasiMonteCarlo
lb = [0.125, 5.0, 5.0]
ub = [1.0, 10.0, 10.0]
n_test = 5
transpose(QuasiMonteCarlo.sample(n_test, lb, ub, GoldenSample()))

We get the following results:

With QuasiMonteCarlo@0.2.16:

 0.192681  5.96681  6.29918
 0.697863  9.43361  5.09836
 0.328044  7.90042  8.89754
 0.833226  6.36723  7.69671
 0.463407  9.83403  6.49589

With QuasiMonteCarlo@0.2.19 (note that the first record is the same as the lower bound):

 0.125     5.0      5.0
 0.841776  8.35522  7.7485
 0.683552  6.71044  5.497
 0.525328  5.06565  8.24551
 0.367104  8.42087  5.99401

@vikram-s-narayan
Copy link
Author

This issue may partly be caused by what appears to be a minor indexing issue in Kronecker.jl:

return @. mod(generator * (0:(n - 1))' + g.origin, 1)

should be

return @. mod(generator * (1:(n))' + g.origin, 1)

Also, the source article suggests that a seed of 0.5 works better. This suggestion had been implemented in prior versions but seems to have been removed in 0.2.19?

@ChrisRackauckas
Copy link
Member

@ParadaCarleton can you take a look at this?

@ParadaCarleton
Copy link
Collaborator

@ParadaCarleton can you take a look at this?

Will make a PR today correcting these issues as part of a general refactor (breaking everything into files, fixing names, and some other stuff).

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

No branches or pull requests

3 participants