[MRG + 1] replace mutable default argument in _RidgeGCV #4376
Conversation
|
Sorry I did a mistake. The numpy array are mutable as well. Maybe we should stop this push. Anyway this should not influence the library at all. |
|
Well, you could use a tuple as you said. It would be good to add a test, though. |
|
Why did you close this? |
|
I was going to open a new one with the tuple instead of array. Is this not the correct procedure? |
|
Sorry I did not know that was possible to update the branch during the pull. I reopen the pull request. I hope that this does not lead to confusion. Regarding the test I need some help. I will ask on the mail list. |
|
You can ask here ;) |
|
I think this looks good to merge. |
| fit_intercept=True, normalize=False, scoring=None, | ||
| score_func=None, loss_func=None, cv=None, gcv_mode=None, | ||
| store_cv_values=False): | ||
| self.alphas = alphas | ||
| self.alphas = np.asarray(alphas) |
There was a problem hiding this comment.
This line mutates the init and we shouldn't.
There was a problem hiding this comment.
argh indeed. this needs to go into the fit method.
|
Whenever my remarks is addressed, +1! |
|
Sorry I need a clarification. Let me know! |
|
Yes, we should never modify anything in the |
|
I have replaced the self.alphas value in fit. I think this make the code more clear. Otherwise I can define a local variable alphas_loc = np.asarray(self.alphas) and replace all the occurencies of self.alphas with alphas_loc in the fit method. I do not know which approach is preffered. |
|
the second approach, though you can just call the variable |
|
in the linereader function it was quiet difficult to change the default value. I have been able to replace it but I am not happy of the quality of the solution. Maybe it can be improved |
| @@ -847,8 +847,10 @@ def fit(self, X, y, sample_weight=None): | |||
| ------- | |||
| self : Returns self. | |||
| """ | |||
| alphas = np.asarray(self.alphas) | |||
There was a problem hiding this comment.
the tests are passed also without that line. The problem is that the default argiment was a numpy array. If you want I can delete it and just use self.alphas in all the method.
There was a problem hiding this comment.
The problem is that the default argiment was a numpy array.
Have I missed something? It seems to be a list.
There was a problem hiding this comment.
check https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/linear_model/ridge.py line 832. The default argument for init was a numpy array. The code works with both self.alphas everywhere or with alphas=np.array(self.alphas). Let me know what version should I keep
There was a problem hiding this comment.
I would go with the least number of lines to modify and go with the tuple.
Though, the doc says that it accepts (only) numpy array. http://scikit-learn.org/dev/modules/generated/sklearn.linear_model.RidgeClassifierCV.html#sklearn.linear_model.RidgeClassifierCV From a user perpsective, I would expect array-like for alphas.
In that case, I would add a regression support and add support for array-like.
There was a problem hiding this comment.
ok then I think the last updated version should be ok. I am using tuples as default value and we expect the users to use np.array. I think that this should work well. Unless I am not missing something. In that case explain me :-)
There was a problem hiding this comment.
In that case, I would add a regression support and add support for array-like.
I mean. I would add a test for this. By the way, we should update the documentation.
There was a problem hiding this comment.
I am afraid that you have to help me with the test. I am not sure of what to test and how to do it.
There was a problem hiding this comment.
For instance, you could test that passing a tuple of alpha, a numpy array of alpha or a list of alpha give similar results.
There was a problem hiding this comment.
ok I have added a test. Let me know if you think that this is fine.
… casted to a numpy array anymore
…ha value in ridgeCV
|
I think that this conflict can be caused by the test. My understanding is that to solve it I have to write the original sklearn repository. I think that only a moderator can do that. |
|
Hi, Let me know if this is fine. Sorry for the confusion I am starting to understand git and the next pull will hopefully be less painful |
|
when you do rebase -i, you can mark all but the first commit as |
# The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 3 commits. # The first commit's message is: # This is a combination of 4 commits. # The first commit's message is: # This is a combination of 18 commits. # The first commit's message is: replace mutable default argument in _RidgeGCV # This is the 2nd commit message: use tuples instead of mutable default values # This is the 3rd commit message: use tuples instead of mutable default values # This is the 4th commit message: redefinition of alphas is removed from init and added to fit method # This is the 5th commit message: use tuples instead of mutable default values # This is the 6th commit message: joblib is restored to its original version. In ridge.py alphas is not casted to a numpy array anymore # This is the 7th commit message: collections library not required anymore and os it is removed # This is the 8th commit message: use tuples instead of mutable default values # This is the 9th commit message: conflict solved # This is the 10th commit message: update documentation # This is the 11th commit message: documentation updated # This is the 12th commit message: use tuples instead of mutable default values # This is the 13th commit message: solve conflict # This is the 14th commit message: up # This is the 15th commit message: conflict solved # This is the 16th commit message: conflict solved # This is the 17th commit message: remose white lines # This is the 18th commit message: delete white lines # This is the 2nd commit message: use tuples instead of mutable default values # This is the 3rd commit message: use tuples instead of mutable default values # This is the 4th commit message: redefinition of alphas is removed from init and added to fit method # This is the 2nd commit message: conflict solved # This is the 3rd commit message: use tuples instead of mutable default values # This is the 2nd commit message: documentation updated # This is the 2nd commit message: use tuples instead of mutable default values # This is the 2nd commit message: redefinition of alphas is removed from init and added to fit method # This is the 2nd commit message: conflict solved # This is the 2nd commit message: use tuples instead of mutable default values # This is the 2nd commit message: redefinition of alphas is removed from init and added to fit method
# The first commit's message is: conflict solved # This is the 2nd commit message: documentation updated # This is the 3rd commit message: remose white lines # This is the 4th commit message: delete white lines
|
I did rebase again. The number of commits now is lower. My understanding is that I can not have a single commit for certain addition. I hope that this if fine now. |
|
Changes Unknown when pulling fb11bc4 on DonBeo:my-feature into * on scikit-learn:master*. |
[MRG + 1] Prevent nose from using docstring to name the tests in results
|
Can you please look at the commit count shown a the top? It shows 97 commits. |
[MRG+2] FIX make StandardScaler & scale more numerically stable
# The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 3 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 3 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 3 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: documentation updated use tuples instead of mutable default values solve conflict up conflict solved conflict solved remose white lines delete white lines use tuples instead of mutable default values use tuples instead of mutable default values redefinition of alphas is removed from init and added to fit method conflict solved use tuples instead of mutable default values documentation updated use tuples instead of mutable default values redefinition of alphas is removed from init and added to fit method conflict solved use tuples instead of mutable default values redefinition of alphas is removed from init and added to fit method conflict solved documentation updated remose white lines delete white lines update documentation replace mutable default argument in _RidgeGCV # This is the 2nd commit message: redefinition of alphas is removed from init and added to fit method # This is the 2nd commit message: use tuples instead of mutable default values # This is the 3rd commit message: conflict solved # This is the 2nd commit message: documentation updated # This is the 2nd commit message: remose white lines # This is the 3rd commit message: delete white lines # This is the 2nd commit message: redefinition of alphas is removed from init and added to fit method # This is the 2nd commit message: conflict solved # This is the 3rd commit message: use tuples instead of mutable default values # This is the 2nd commit message: documentation updated # This is the 2nd commit message: redefinition of alphas is removed from init and added to fit method # This is the 2nd commit message: conflict solved # This is the 2nd commit message: redefinition of alphas is removed from init and added to fit method # This is the 2nd commit message: documentation updated # This is the 3rd commit message: remose white lines # This is the 4th commit message: delete white lines
[MRG+1] FIX LDA(solver="lsqr"): make sure the right error is raised on transform
|
sorry, I'd be happy to help you, but I just missed you on IRC. you can ping me as amueller, then something will bounce on my desktop ;) |
|
Changes Unknown when pulling 6d2c599 on DonBeo:my-feature into * on scikit-learn:master*. |
|
#4713 merged |
This is a very small change to make me familiar with the repository and the git system. The mutable default value is replaced with a numpy array. This code passes al the tests.