[MRG] Raise descriptive ValueError if number of samples equals number of classes in Linear Discriminant Analysis#12391
Conversation
|
Please give your PR a more descriptive title
|
|
The PR *description* should contain "Fixes #xxxx"
|
|
Could you please also add a test? You also have a PEP8 issue, hence travis failing. |
6a09189 to
3c1f7a1
Compare
| X = np.zeros((no_classes, no_classes)) | ||
| y = np.zeros((no_classes, 1)) | ||
| clf = LinearDiscriminantAnalysis(solver="svd") | ||
| assert_raises(ValueError, clf.fit, X, y) |
There was a problem hiding this comment.
| assert_raises(ValueError, clf.fit, X, y) | |
| with pytest.raises(ValueError, match="The number of samples must be more"): | |
| clf.fit(X, y) |
There was a problem hiding this comment.
ah nice, tests the message too to make sure that its actually THAT ValueError being raised
There was a problem hiding this comment.
yes, and since the adoption of pytest, pytest.... are generally preferred to custom assert_... ones.
There was a problem hiding this comment.
thanks for your advice!
706a3df to
d0f62c8
Compare
|
Tests are failing |
|
Please append commits rather than amending and force-pushing. It makes it hard for us to review when we aren't notified that a commit has been added and we can't see the diff |
jnothman
left a comment
There was a problem hiding this comment.
Since classes_ is set as unique values in y, it seems a little strange to have the message state the number of samples and classes when they must be equal
sklearn/discriminant_analysis.py
Outdated
| "Currently, you have {} samples and {} " | ||
| "classes.".format(n_samples, n_classes)) | ||
|
|
||
|
|
There was a problem hiding this comment.
please have only one empty new line. Why wouldn't flake8 not fail on this on travis?
|
This case should also be handled for the other solvers. Currently eigen raises an error, while lsqr gives a warning. |
The lsqr and eigen solvers also will fail if the number of samples equals the number of classes, so this commit moves the error raise to cover all cases.
|
@jnothman @adrinjalali thanks for your patience. How is it looking? |
jnothman
left a comment
There was a problem hiding this comment.
Otherwise LGTM. I don't know if it's helpful to users to put this in our change log...?
| assert_almost_equal(c_s, c_s.T) | ||
|
|
||
|
|
||
| def test_raises_value_error_on_same_number_of_classes_and_samples(): |
There was a problem hiding this comment.
Use pytest.mark.parameterize to test this for each solver I suppose
|
@adrinjalali what is the next step with getting this merged then? I think I've misunderstood the process a bit... |
|
@eddbrown an unwritten part of the process is the casual (read usual, normal) patience required for a core developer to do the next action ;) let it be a review, response to a question, merge, etc. Your PR now is in a state to be merged, and I guess it will be soon (I'm not somebody with permissions to do a merge). Now you can just sit back and relax until it happens, or go on and find another issue to investigate ;) |
I'm pretty sure I mentioned patience somewhere in the contributor docs in the last couple of years... ? Thanks @eddbrown |
…f classes in Linear Discriminant Analysis (scikit-learn#12391)
…f classes in Linear Discriminant Analysis (scikit-learn#12391)
…f classes in Linear Discriminant Analysis (scikit-learn#12391)
…number of classes in Linear Discriminant Analysis (scikit-learn#12391)" This reverts commit 6cae448.
…number of classes in Linear Discriminant Analysis (scikit-learn#12391)" This reverts commit 6cae448.
…f classes in Linear Discriminant Analysis (scikit-learn#12391)
Fixes #12374
Before: Failed with division by zero error if no_classes == no_sample in Linear Discriminant Analysis.
After: This PR raises a more descriptive ValueError, explaining the problem.