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

Added seed for random numbers generators #62

Closed
wants to merge 11 commits into from
Closed

Conversation

echo66
Copy link

@echo66 echo66 commented Oct 10, 2018

Right now, when using the sampling methods, in order to get the same result everytime, we need to do the following things:

  1. invoke np.random.seed(seed_value)
  2. invoke random.set_state(random_state_tuple)

outside of the function being called (i.e. sample). This is what people call a "leaky abstraction", in software engineering. An easy way to cope with this is to include the seed as a parameter in the sample method. Usually, we just take that seed, use it as an input for something like sklearn.check_random_state and we receive a random numbers generator as an output, using it instead of numpy.random. But since the code for some classes is using functions that allow us to pass the seed or random numbers generator instantiated by us, we need to be a little bit more sophisticated by using numpy.random.get_state, numpy.random.set_state, random.getstate and random.setstate.

echo66 and others added 11 commits October 10, 2018 14:27
Added parameter to set the seed for the random numbers generator inside KDEUnivariate.sample.
Added parameter to set the seed for the random numbers generator inside GaussianUnivariate.sample.
Added parameter to set the seed for the random numbers generator inside Bivariate.sample.
Added parameter to set the seed for the random numbers generator inside GaussianMultivariate.sample.
Added seed management to Vine
simplified seed management
Simplified seed management
Simplified seed management
Simplified seed management
* Fixed variable names.
@echo66
Copy link
Author

echo66 commented Oct 10, 2018

I can't understand what is failing in Travis. All those errors seem related to docstrings and rst files I didn't touch.

@csala
Copy link
Contributor

csala commented Oct 10, 2018

Thanks for you contribution @echo66 !

Indeed, being able to pin the random seed within the sampling methods itself seems to be an interesting addition, so we really appreciate your interest in contributing these changes.

Unfortunately, I'm afraid that your PR does not adhere to our contributing guidelines, which you can find here

Briefly, I suggest the following approach:

  • Cancel the current PR (temporary).
  • Open a GitHub issue explaining what you would like to see changed, why, and how you would implement it. We would love to share our impressions there and be able to briefly discuss any possible discrepancy about the ideal way to implement the changes.
  • Once we have agreed on the implementation, please make sure that:
    • Your implementation corresponds to what has been discussed in the issue.
    • There are unit tests that explicitly validate the changes associated with the issue
    • The changes are well documented, especially if there are API changes, like the new method arguments that this PR seems to add.
    • All the tests and code styling checks pass successfully in all platforms (running make test-all).
  • Once all this is validated, please issue a new PR and we'll gladly review and merge it as soon as we can.

@csala
Copy link
Contributor

csala commented Oct 10, 2018

Regarding the Travis build failure, it seems to be because some of the changed files do not pass the flake8 validation.

You check this locally by running make lint, which will show a nice report about whether the code styling checks pass or not and, if they don't, what things need to be changed.

@echo66
Copy link
Author

echo66 commented Oct 11, 2018

Thanks for the feedback! I will do as you told.

EDIT: I opened #63

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.

None yet

2 participants