[WIP] Storing the best attributes of (non-GridSearch) CV models #6215
Conversation
|
@rvraghav93 Do i need to squash some commits ? |
|
Umm I don't know as it is not yours, ask @MechCoder or @amueller Also please copy the title/description from the other PR |
|
@rvraghav93 Sure. In the meantime, i'll try fixing travis. This one seems interesting 😄 |
7668fbc to
2d66258
Compare
|
@rvraghav93 The tests pass locally. Hope travis passes. BTW, am yet to add test for best_score_ attribute to _BaseRidgeCV. I'm not still clear with writing a test that should fail in master. Could you please help me with that ? |
| rng = rng = np.random.RandomState(42) | ||
| """ | ||
| Test _RidgeCV's store_cv_values attribute. | ||
| """ |
There was a problem hiding this comment.
Could you pl. use comments for tests, see #4432
There was a problem hiding this comment.
Sure. Will check.
|
@rvraghav93 I'm not understanding as to how all the tests passes on my system and travis but not on appveyor. |
|
Appveyor failure is unrelated to your PR. Please ignore |
Not sure I understand your question but since we are adding this parameter only now, don't worry about that failing in master. We need only NRTs to fail in master and pass in the branch (which fixes the regression). |
| @@ -17,7 +17,7 @@ | |||
| from .base import center_data, sparse_center_data | |||
| from ..utils import check_array, check_X_y, deprecated | |||
| from ..utils.validation import check_random_state | |||
| from ..model_selection import check_cv | |||
| from ..cross_validation import check_cv | |||
There was a problem hiding this comment.
The import path "cross_validation" is deprecated. I am curious to know why you changed to using it.
There was a problem hiding this comment.
@GaelVaroquaux Sorry, didn't notice it at the time of rebase. will change it
There was a problem hiding this comment.
@GaelVaroquaux , travis is failing because of this change. I'll look into this
|
I didn't look at the complete PR, but it contains many lines that are probably due to an error in using version control, as they are actually undoing recent changes that have been done in master. These lines needs to be corrected before we can consider merging this PR. |
|
please don't squash commits that are not yours into yours. if there are multiple commits that are from the same author, I think it's ok to squash them into one. |
2d66258 to
3218b5e
Compare
|
@GaelVaroquaux Yeah, those changes are probably because i rebased the old PR. Have made the changes that you've suggested. |
|
@GaelVaroquaux Yeah, those changes are probably because i rebased the
old PR. Have made the changes that you've suggested.
You're going to want to audit all the changes that appear in the PR to
make sure that they are not spurious, and have indeed been authored by
you and not drawn in by a rebase that has gone astray.
|
|
@GaelVaroquaux Yeah, sure. I'll inspect and scrutinize all the files. |
3218b5e to
574a215
Compare
|
I don't see benchmarks on when |
Extension of work on #5498