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.

Sign up for GitHub

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

Jump to bottom

Clean up interface to for specifying seeds to optuna samplers with PEC. #2876

Open
wants to merge 6 commits into
base: devel
Choose a base branch
from
Open

Clean up interface to for specifying seeds to optuna samplers with PEC. #2876

wants to merge 6 commits into from

Conversation

davidt0x
Copy link
Collaborator

@davidt0x davidt0x commented Jan 9, 2024 •
edited
Loading

Some small modifications to PEC\Optuna RNG seeding interface based on suggestions from @jvesely and @jdcpni.

  • Hack was removed that set optuna RandomState object under the hood.
  • Added ability to pass un-instantiated optuna sampler to PECOptimizationFunction and added an argument called optuna_kwargs for specifying any desired sampler construction args. This allows use the PEC initial_seed if it is specified.
  • Added a warning to the user if they have specified a PEC initial_seed with an already instantiated optuna sampler.

Fixes issue #2874

@davidt0x
Fix issue with seeding optuna samplers.
981e6b4 
Figured out a better way to reseed and already
instatiated Optuna sampler. I was overwriting the
RNG state in a hacky way before. I didn't realize there
was a reseed_rng() method on the the base sampler class
that can be called when the seed is re-initialized.
@davidt0x
Switch to QMCSampler instead of CMA-ES
86b7a11 
Test for CMA-ES sampler was actually just falling back
to RandomSampler. I just want to test things work with
another sampler so I switched to QMCSampler because it
also supports 1D search spaces.
@davidt0x
Update optuna pin to latest.
871f960
@davidt0x
Remove hack for setting optuna RNG.
848fe50
@davidt0x
Add warnings about seeding optuna.
caaefc6 
Added support for passing kwargs to un-instantiated
optuna sampler.
@davidt0x
Change type to Type for Python 3.7
54c3840
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2024

This PR causes the following changes to the html docs (ubuntu-latest-3.11-x64):

Binary files docs-base/_images/Composition_XOR_animation.gif and docs-head/_images/Composition_XOR_animation.gif differ

...

See CI logs for the full diff.

@coveralls
Copy link

coveralls commented Jan 9, 2024 •
edited
Loading

Coverage Status

coverage: 84.931% (+0.06%) from 84.871%
when pulling 54c3840 on fix_optuna_rng
into f062be4 on master.

@jvesely
Copy link
Collaborator

jvesely commented Jan 10, 2024

Should this go straight to master?

I think it's better to drop 871f960 ("Update optuna pin to latest.") from this PR.
The constraint is not a pin, just an upper bound and dependabot produces more informative commits with links to release notes and a summary of changes between versions.
Alternatively you can cherry-pick 0d10fea to include that information.

@davidt0x davidt0x changed the base branch from master to devel January 10, 2024 21:48
@davidt0x
Copy link
Collaborator Author

@jvesely, whoops that was a mistake about going straight to master. Thanks for catching. I will drop the change of the pin as well.

@jvesely
Copy link
Collaborator

jvesely commented Jan 11, 2024

One more nit. Will the existing two tests that use instanced samplers issue warnings? If so, should the test check that the warnings are emitted in those two cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@davidt0x @coveralls @jvesely