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 BaseSeedable class. #39

Merged
merged 5 commits into from
Mar 20, 2023
Merged

Add BaseSeedable class. #39

merged 5 commits into from
Mar 20, 2023

Conversation

marcofavorito
Copy link
Contributor

Proposed changes

This is the base class for all objects that need to keep a random state.

In particular, the interface provides the following features:

  • it is able to accept a random seed from the client;
  • it allows the client reset the random seed;
  • it provides a getter to the internal random generator;
  • it allows to sample a random seed (in the range [0, 2^32 - 1]).

Fixes

n/a

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • I am making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

Further comments

n/a

@marcofavoritobi marcofavoritobi requested a review from AldoGl March 16, 2023 12:32
marcofavoritobi and others added 2 commits March 18, 2023 20:42
This is the base class for all objects that need to keep a random state.

In particular, the interface provides the following features:
- it is able to accept a random seed from the client;
- it allows the client reset the random seed;
- it provides a getter to the internal random generator;
- it allows to sample a random seed (in the range [0, 2^32 - 1]).
This is a breaking change, since who used get_random_seed directly from
black_it.utils.base would get an error.

This must be documented in the release notes of the next release.
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2023

Codecov Report

Merging #39 (45eda20) into main (3fd8000) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   96.82%   96.79%   -0.03%     
==========================================
  Files          30       31       +1     
  Lines        1512     1499      -13     
==========================================
- Hits         1464     1451      -13     
  Misses         48       48              
Impacted Files Coverage Δ
black_it/utils/base.py 100.00% <ø> (ø)
black_it/calibrator.py 97.29% <100.00%> (-0.19%) ⬇️
black_it/samplers/base.py 100.00% <100.00%> (ø)
black_it/samplers/halton.py 100.00% <100.00%> (ø)
black_it/samplers/r_sequence.py 100.00% <100.00%> (ø)
black_it/samplers/random_uniform.py 100.00% <100.00%> (ø)
black_it/utils/seedable.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@marcofavoritobi marcofavoritobi marked this pull request as ready for review March 19, 2023 10:40
Copy link
Contributor

@AldoGl AldoGl left a comment

Choose a reason for hiding this comment

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

LGTM, I provide just a couple of comments

Initialize the sampler.

Args:
random_state: the internal state of the sampler, fixing this numbers the sampler behaves deterministically
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this description could be changed to "[...] fixing this numbers the object (either a sampler or a scheduler) behaves deterministically"

Copy link
Contributor

Choose a reason for hiding this comment

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

OK Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

(It could be a calibrator too)

Copy link
Contributor

Choose a reason for hiding this comment

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

right!

def _set_random_state(self, random_state: Optional[int]) -> None:
"""Set the random state, private use."""
self.__random_state = random_state
self.__random_generator = default_rng(self.random_state)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this is a naive comment, but shouldn't this be self.__random_generator = default_rng(random_state)? If we use self.random_state in the argument I am not sure we would get the behaviour we want

Copy link
Contributor

Choose a reason for hiding this comment

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

self.random_state is the property getter, that reads from self.__random_state, which is set the line above

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this makes sense, thanks

marcofavoritobi pushed a commit that referenced this pull request Mar 20, 2023
@marcofavoritobi marcofavoritobi merged commit 4624af4 into main Mar 20, 2023
@marcofavoritobi marcofavoritobi deleted the feat/seedable branch March 20, 2023 16:36
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.

4 participants