Warning resolve convergence warning by reducing the default penalty by rwaithera · Pull Request #14143 · scikit-learn/scikit-learn

rwaithera · 2019-06-22T08:22:49Z

Added a lower penalty to LinearSVC(). Made C=0.01 instead of the default 1. This resolves the Convergence Warning.

#WiMLDS_Nairobi
@Darthvaderkenya

adrinjalali · 2019-06-22T08:25:00Z

Please also say something like (related to #14117), when working on the issue :)

rwaithera

related to #14117

adrinjalali · 2019-06-22T09:14:03Z

two points which may be useful:

Fixes #(number of the issue) will cause the issue to be closed after the PR is merged. Since this only fixes one of the issues in that main issue, it should be said "related to #(issue number)
The "fixes" or "closes" magic words work if you put them in the description of the PR, not as a comment :) you can edit the description and add it there.

adrinjalali · 2019-06-22T09:16:08Z

Could you also paste the output of the example before and after the change?

rwaithera · 2019-06-22T09:30:35Z

Could you also paste the output of the example before and after the change?

Before
The best_index_ is 3
The n_components selected is 8
The corresponding accuracy score is 0.84

After
The best_index_ is 3
The n_components selected is 8
The corresponding accuracy score is 0.85

adrinjalali · 2019-06-22T10:02:33Z

thanks, the output looks good, could you also say how long it takes to run before and after the change?

rwaithera · 2019-06-22T10:11:48Z

thanks, the output looks good, could you also say how long it takes to run before and after the change?

Before, the example took 20.4s to run.
After including a penalty of 0.01 the code run in 6.9s.

Thanks Adrin for your views!

adrinjalali

Awesome, LGTM, thanks @rwaithera :)

thomasjpfan · 2019-06-23T01:47:38Z

On master the example output was:

The best_index_ is 2
The n_components selected is 6
The corresponding accuracy score is 0.80

The docstring says:

The balanced case is when n_components=6 and accuracy=0.80,
which falls into the range within 1 standard deviation of the best accuracy
score.

NicolasHug · 2019-06-24T14:58:47Z

Please try n_components=7 and see if the docstring is respected?

glemaitre · 2019-07-02T08:14:20Z

If it is fine with everyone, I propose the following:

param_grid = {
    'reduce_dim__n_components': [6, 8, 10, 12, 14]
}

It leads to the following figure and we can update the description accordingly. It enforces what is discuss in the description of the example.

@rwaithera can you update the example or to you want me to quickly address those and merge your PR?

rwaithera · 2019-07-02T10:01:22Z

Please try n_components=7 and see if the docstring is respected?

I get the following after adding 7 into the reduce_dim__n_components list:

The best_index_ is 3
The n_components selected is 7
The corresponding accuracy score is 0.82

rwaithera · 2019-07-02T10:04:03Z

If it is fine with everyone, I propose the following:
param_grid = {
    'reduce_dim__n_components': [6, 8, 10, 12, 14]
}
It leads to the following figure and we can update the description accordingly. It enforces what is discuss in the description of the example.

@rwaithera can you update the example or to you want me to quickly address those and merge your PR?

That increases the accuracy. Nice @adrinjalali .

I will update the cahnges.

glemaitre · 2019-07-02T10:46:32Z

The important message here is that n_components=10 is good enough because we are in the best_score + 1 std. dev.

rwaithera · 2019-07-02T12:12:07Z

The important message here is that n_components=10 is good enough because we are in the best_score + 1 std. dev.

Ah! I see now. It is all clear what everyone meant by within 1 std from the best accuracy score.

Thanks @glemaitre!

reshamas · 2019-07-12T18:36:15Z

@rwaithera
Checking in on PR. Do you have to do something for this, or are we waiting on a reviewer?

@adrinjalali
If we are waiting on a reviewer, are you able to find someone? It would be great to have PR merged in by July 22, which is one month post-sprint. Thanks.

cc: @Mariam-ke

adrinjalali · 2019-07-12T20:24:46Z

I think there are still changes which need to be applied by @rwaithera , and then @glemaitre can accept the PR

reshamas · 2019-07-12T20:57:32Z

@Mariam-ke
Can you email Ruth (@rwaithera) and ask her to please follow-up? It has been 10 days since the last activity, and after 14 days, the PR can go back in the pool.

To decide whether an inactive PR is stalled, ask the contributor if she/he plans to continue working on the PR in the near future. Failure to respond within 2 weeks with an activity that moves the PR forward suggests that the PR is stalled and will result in tagging that PR with “help wanted”.

As an FYI: there are sprints going on in Austin, TX this weekend, and then the NYC sprint coming up in August, so this can go to someone else.

Thanks.

NicolasHug · 2019-07-13T13:58:42Z

Marking this available for the Austin sprint (hope that's OK. I think the 14 days rule is too strict when there are upcoming sprints).

As far as I can tell all this PR needs is to update the docstring description above

reshamas · 2019-07-13T14:08:56Z

@NicolasHug That is fine. Would be good to see this completed.

wendyhhu · 2019-07-13T15:28:51Z

Hi folks, I'm working on this issue.

rwaithera · 2019-07-13T16:00:08Z

@rwaithera
Checking in on PR. Do you have to do something for this, or are we waiting on a reviewer?

@adrinjalali
If we are waiting on a reviewer, are you able to find someone? It would be great to have PR merged in by July 22, which is one month post-sprint. Thanks.

cc: @Mariam-ke

@reshamas I submitted the suggestion @glemaitre gave. Please @glemaitre , review the changes.

Thanks!

NicolasHug · 2019-07-13T17:40:09Z

@rwaithera the docstring at the beginning of the example needs an update to be consistent with the current changes (see the changes proposed in #14329 by @wendyhhu).

Would you be able to submit them soon? Thanks!

rwaithera · 2019-07-15T06:59:54Z

@rwaithera the docstring at the beginning of the example needs an update to be consistent with the current changes (see the changes proposed in #14329 by @wendyhhu).

Would you be able to submit them soon? Thanks!

@NicolasHug I see the changes were updated in the documentation.

NicolasHug · 2019-07-15T12:20:37Z

@rwaithera, what I mean is that we need you to apply the same changes as in #14329 for your PR to get merged.

…ding n_components

rwaithera · 2019-07-15T13:46:05Z

@rwaithera, what I mean is that we need you to apply the same changes as in #14329 for your PR to get merged.

I have applied the changes @NicolasHug

NicolasHug · 2019-07-16T13:39:04Z

examples/model_selection/plot_grid_search_refit_callable.py


 The figure shows the trade-off between cross-validated score and the number
-of PCA components. The balanced case is when n_components=6 and accuracy=0.80,
+of PCA components. The balanced case is when n_components=12 and accuracy=0.90,


@rwaithera thanks, we're almost there. Just need to update the numbers:

Suggested change

of PCA components. The balanced case is when n_components=12 and accuracy=0.90,

of PCA components. The balanced case is when n_components=10 and accuracy=0.88,

adrinjalali · 2019-07-18T13:10:54Z

@rwaithera some final changes you need to make, should be then ready for a merge :)

NicolasHug

Thank you for your work @rwaithera

rwaithera · 2019-07-19T05:26:24Z

Thank you for your work @rwaithera

Thank you for your guidance @NicolasHug

resolve convergence warning by reducing the default penalty

8e4c068

adrinjalali added the Sprint label Jun 22, 2019

rwaithera commented Jun 22, 2019

View reviewed changes

rwaithera closed this Jun 22, 2019

rwaithera reopened this Jun 22, 2019

Merge remote-tracking branch 'upstream/master' into my-feature

fb34c8f

adrinjalali approved these changes Jun 22, 2019

View reviewed changes

glemaitre self-requested a review July 2, 2019 07:48

change reduce_dim__n_components list elements

64fb71a

wendyhhu mentioned this pull request Jul 13, 2019

[MRG] fix convergence warning for plot_grid_search_refit_callable.py #14329

Closed

Ruth Waithera Wachira added 2 commits July 15, 2019 16:41

update the docstring to reflect the best accuracy score and correspon…

1e2254e

…ding n_components

Merge remote-tracking branch 'upstream/master' into my-feature

876afc7

NicolasHug reviewed Jul 16, 2019

View reviewed changes

change docustring to read 10 n_components and 0.88 accuracy

bf62872

NicolasHug approved these changes Jul 18, 2019

View reviewed changes

NicolasHug merged commit b4f2bf4 into scikit-learn:master Jul 18, 2019

amueller mentioned this pull request Aug 7, 2019

[MRG] Ensure model convergence in Balancing Model Complexity example #14333

Closed

	of PCA components. The balanced case is when n_components=12 and accuracy=0.90,
	of PCA components. The balanced case is when n_components=10 and accuracy=0.88,

Uh oh!

Conversation

rwaithera commented Jun 22, 2019

Uh oh!

adrinjalali commented Jun 22, 2019

Uh oh!

rwaithera left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Jun 22, 2019

Uh oh!

adrinjalali commented Jun 22, 2019

Uh oh!

rwaithera commented Jun 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali commented Jun 22, 2019

Uh oh!

rwaithera commented Jun 22, 2019

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Jun 23, 2019

Uh oh!

NicolasHug commented Jun 24, 2019

Uh oh!

glemaitre commented Jul 2, 2019

Uh oh!

rwaithera commented Jul 2, 2019

Uh oh!

rwaithera commented Jul 2, 2019

Uh oh!

glemaitre commented Jul 2, 2019

Uh oh!

rwaithera commented Jul 2, 2019

Uh oh!

reshamas commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali commented Jul 12, 2019

Uh oh!

reshamas commented Jul 12, 2019

Uh oh!

NicolasHug commented Jul 13, 2019

Uh oh!

reshamas commented Jul 13, 2019

Uh oh!

wendyhhu commented Jul 13, 2019

Uh oh!

rwaithera commented Jul 13, 2019

Uh oh!

NicolasHug commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwaithera commented Jul 15, 2019

Uh oh!

NicolasHug commented Jul 15, 2019

Uh oh!

rwaithera commented Jul 15, 2019

Uh oh!

NicolasHug Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Jul 18, 2019

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

rwaithera commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rwaithera left a comment •

edited

Loading

rwaithera commented Jun 22, 2019 •

edited

Loading

reshamas commented Jul 12, 2019 •

edited

Loading

NicolasHug commented Jul 13, 2019 •

edited

Loading