Skip to content
This repository was archived by the owner on Feb 28, 2024. It is now read-only.

[MRG] Refactor Sobol' API #955

Merged
holgern merged 5 commits into from
Sep 29, 2020
Merged

[MRG] Refactor Sobol' API #955

holgern merged 5 commits into from
Sep 29, 2020

Conversation

@tupui
Copy link
Contributor

@tupui tupui commented Sep 28, 2020 •
edited
Loading

Address #953

  • Fix typo for Sobol' and extend documentation
  • Raise if ones tries to skip n != 2**m points
  • Warn about improper sampling (not a power of 2)
  • Default to randomize (better convergence properties)
  • Base Sobol' and Halton have zero as first point

@pep8speaks
Copy link

pep8speaks commented Sep 28, 2020 •
edited
Loading

Hello @tupui! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-29 07:21:27 UTC

@tupui tupui changed the title Refactor skip in Sobol, add warnings and improve doc Refactor Sobol' API Sep 28, 2020
@tupui tupui marked this pull request as ready for review September 29, 2020 07:34
@tupui
Copy link
Contributor Author

tupui commented Sep 29, 2020

FYI, on top of the enhancements, I fixed some CI inconsistencies (mismatching python versions) and some tests in test_plots.py (wrong tolerance setting).

@tupui tupui changed the title Refactor Sobol' API [MRG] Refactor Sobol' API Sep 29, 2020
holgern
holgern approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@holgern holgern left a comment

Choose a reason for hiding this comment

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

Thank you for your work, everything looks good to me!

@holgern holgern merged commit 9334d50 into scikit-optimize:master Sep 29, 2020
@tupui
Copy link
Contributor Author

tupui commented Sep 29, 2020

@holgern Thanks for handling this so quickly, really appreciate it 😃

@tupui tupui deleted the refactor_sobol branch August 30, 2022 13:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@holgern holgern holgern approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tupui @pep8speaks @holgern