[MRG] Remove random_seed in fit and use the one in init #224
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wdevazelhes
commented
Jun 25, 2019
| @@ -866,21 +866,6 @@ def test_iris(self): | |||
| csep = class_separation(rca.transform(self.iris_points), self.iris_labels) | |||
| self.assertLess(csep, 0.25) | |||
|
|
|||
| def test_feature_null_variance(self): | |||
There was a problem hiding this comment.
while I was at it I removed this test because it tested pca_comps which is now deprecated. It failed (when it passed before) probably because of the random_seed, which may change because of this PR
wdevazelhes
commented
Jun 25, 2019
test/metric_learn_test.py
Outdated
| @@ -861,7 +861,8 @@ def test_deprecation_num_dims_lfda(num_dims): | |||
|
|
|||
| class TestRCA(MetricTestCase): | |||
| def test_iris(self): | |||
| rca = RCA_Supervised(n_components=2, num_chunks=30, chunk_size=2) | |||
| rca = RCA_Supervised(n_components=2, num_chunks=30, chunk_size=2, | |||
There was a problem hiding this comment.
I set the random seed here to pass the test, because the random_seed may be different than before.
|
LGTM. There is just some conflicts with master to address before we can merge |
bellet
approved these changes
Jul 3, 2019
added 2 commits
July 3, 2019 18:05
# Conflicts: # metric_learn/itml.py # metric_learn/lsml.py # metric_learn/mmc.py # metric_learn/rca.py # metric_learn/sdml.py
|
There was a small pb with the merge but it should be fixed right now, if it's green I think we can merge |
metric_learn/constraints.py
Outdated
| @@ -46,7 +49,8 @@ def positive_negative_pairs(self, num_constraints, same_length=False, | |||
| return a, b, c, d | |||
|
|
|||
| def _pairs(self, num_constraints, same_label=True, max_iter=10, | |||
| random_state=np.random): | |||
| random_state=None): | |||
| random_state = check_random_state(random_state) | |||
There was a problem hiding this comment.
It's probably safe to remove this check, as all callers of _pairs should have done it already.
perimosocordiae
approved these changes
Jul 3, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #171
This PR replaces the
random_seedinfit, by therandom_seedininit.Note that I also updated the functions
positive_negative_pairs,_pairs, andadjacency_matrixto be more like scikit-learn (i.e. have a therandom_stateparameter to beNoneby default), but I don't think it's necessary to put aChangedBehaviourWarning or aDeprecationWarning, since those are util functions, that were not previously documented, (and probably less used than*_Supervisedversions of algorithms), and what is more the previous behaviour will still work (givingnp.randomor anyRandomState), what do you think ?