Skip to content

[MRG] Changed self.rng to private (self.rng_) in sklearn/gaussian_process/g… #7766

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
Closed
wants to merge 3 commits into from
Closed

[MRG] Changed self.rng to private (self.rng_) in sklearn/gaussian_process/g… #7766

wants to merge 3 commits into from

Conversation

tayyabpw
Copy link

Reference Issue

Fixes #7752

What does this implement/fix? Explain your changes.

Changes self.rng to private (self.rng_) as stated in the issues page

Any other comments?

…pr.py

@tayyabpw tayyabpw changed the title Changed self.rng to private (self.rng_) in sklearn/gaussian_process/g… [MRG] Changed self.rng to private (self.rng_) in sklearn/gaussian_process/g… Oct 27, 2016
@amueller
Copy link
Member

Is there a reason to have it as an attribute at all?

@tayyabpw
Copy link
Author

I don't feel that it's absolutely necessary but in #7752, it seemed like @amueller wanted it to be rng_ and all occurrences of rng should be changed to rng_.

@amueller
Copy link
Member

I changed my mind. If it's not necessary, we should not set it.

@amueller
Copy link
Member

hm do we need a deprecation, though? hm

@MechCoder
Copy link
Member

Not if this fixed in 0.18.1 right?

@dalmia
Copy link
Contributor

dalmia commented Nov 11, 2016

@amueller It may be required as an attribute in case it's required to reproduce a certain result.

dalmia
dalmia reviewed Nov 11, 2016
View reviewed changes
sklearn/gaussian_process/gpr.py
@@ -161,7 +161,7 @@ def fit(self, X, y):
else:
self.kernel_ = clone(self.kernel)

self.rng = check_random_state(self.random_state)
self.rng_ = check_random_state(self.random_state)
Copy link
Contributor

@dalmia dalmia Nov 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@tayyabpw You need to add 'rng_' in the 'Attributes' section in the docstring above.

@tayyabpw
Copy link
Author

tayyabpw commented Nov 11, 2016 •
edited
Loading

Will change in the next 3 hours. Thanks

@tayyabpw
Copy link
Author

Added Attribute to docstring. There was another self.rng in fit() function that I had missed out. Fixed that.

Please review.

dalmia
dalmia suggested changes Nov 12, 2016
View reviewed changes
sklearn/gaussian_process/gpr.py
@@ -128,6 +128,8 @@ def optimizer(obj_func, initial_theta, bounds):
log_marginal_likelihood_value_ : float
The log-marginal-likelihood of ``self.kernel_.theta``

rng_ : numpy.RandomState

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to explain what this attribute is. Look at the above declarations. The user needs to know what this stands for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe like this:

rng_ : numpy.RandomState
   random_state used at the time of initialization

@dalmia
Copy link
Contributor

dalmia commented Nov 16, 2016

@amueller Please give your views in this regard. Would you prefer keeping it as an attribute or better have it removed?
Thanks.

@amueller
Copy link
Member

I think we should remove it, but we need to deprecate it.

@dalmia
Copy link
Contributor

dalmia commented Nov 29, 2016 •
edited
Loading

@tayyabpw As @amueller suggested, you need to deprecate the attribute. Please refer the docs on how to deprecate an attribute and let me know if you are available to work on this further. You need to add the deprecation, undo your changes and add backward compatibility for the deprecation.

@dalmia
Copy link
Contributor

dalmia commented Dec 1, 2016

@amueller Since @tayyabpw doesn't seem up for it, should I make a quick PR making the deprecation?

@amueller
Copy link
Member

amueller commented Dec 1, 2016

Give @tayyabpw a couple of days, there's not real urgency here. There's plenty of issues to go around ;)

@amueller amueller mentioned this pull request Dec 5, 2016
Closed
@dalmia
Copy link
Contributor

dalmia commented Dec 7, 2016

@amueller @tayyabpw doesn't seem to be active. Do you feel I should move ahead with this?
Thanks.

@amueller
Copy link
Member

amueller commented Dec 7, 2016

@dalmia sure, go ahead

@amueller amueller added Easy Well-defined and straightforward way to resolve Stalled good first issue Easy with clear instructions to resolve help wanted labels Aug 21, 2018
@abenbihi
Copy link
Contributor

@ogrisel, This has been solved by #7846.
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/gaussian_process/gpr.py#L183

So this PR can be closed.

@rth
Copy link
Member

rth commented Feb 25, 2019

Thanks for checking @AssiaBen !

@rth rth closed this Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalmia dalmia dalmia requested changes

Assignees
No one assigned
Labels
Easy Well-defined and straightforward way to resolve good first issue Easy with clear instructions to resolve help wanted Stalled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rng attribute is set in sklearn.gaussian_process.GaussianProcessRegressor at fit time
6 participants
@tayyabpw @amueller @MechCoder @dalmia @abenbihi @rth