[FIX] sgd: Change deprecated n_iter to max_iter

[smiks]

Issue

SGDClassificationLearner deprecated parameter (n_iter). Fixes #2917.

Description of changes

Deprecation fix: changed n_iter to max_iter (SGDClassifier)

Includes

• Code changes
• Tests
• Documentation

[CLAassistant]

All committers have signed the CLA.

[ajdapretnar]

Default in sklearn is 5. Is there a particular reason to have it at 10?

[smiks]

No particular reason. Sklearn will change it to 1000 from 0.21. I decided for 10 only to see if I changed everything correctly (testing widget in Orange).

[pavlin-policar]

Please implement a migrate_settings method. Without it, old workflows won't use the old n_iter value as the max_iter value while their semantics are essentially the same.

[smiks]

I don't know how to do it.
Is this correct (added in class OWSGD)?

    @classmethod
    def migrate_settings(cls, settings_, version):
        if version < 2:
            settings_["max_iter"] = settings_.get("n_iter", 5)

[pavlin-policar]

That should do the trick. Perhaps delete the setting n_iter as well to clean up the old settings? You can do this in one line settings_["max_iter"] = settings_.pop("n_iter", 5).

[codecov-io]

Codecov Report

Merging #2920 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2920      +/-   ##
=========================================
+ Coverage    82.1%   82.1%   +<.01%     
=========================================
  Files         328     328              
  Lines       56262   56265       +3     
=========================================
+ Hits        46194   46198       +4     
+ Misses      10068   10067       -1

[lanzagar]

Thanks for the contribution. Looks good, just 2 things:

1. SGDRegressionLearner should also be fixed in the same way (we have a common widget for both). It is in Orange/regression/linear.py
2. We should probably add a control in the widget to set the tol parameter. It is not critical for this PR since the previous functionality is still the same. But if you don't add it now, please open an Issue that we should consider adding it before sklearn 0.21 (seems setting tol instead of max_iter will be the preferred way in the fututre).

(Also, the linting and change default back commits should be squashed into the first one at the end)

[smiks]

I'll try to do both (tol parameter and fix for SGDRegressionLearner).
I only have to fix learner (widget is ok - in case I don't add tol parameter), right?

[lanzagar]

Yes, the SGD widget calls the regression or classification learner depending on the type of data it gets.
It now already uses max_iter, so just the regression SGD class needs to be updated to match it.

[lanzagar]

It was hard to fall back to the old functionality (of setting just the number of iterations) without being able to set tol to None - I guess that is why you also removed migrate settings? I added a checkbox for tol and brought back migration.
You can double check my commit to make sure everything is still OK, then I think this is ready for merging.

[smiks]

I'll check it. It is my first PR for Orange so I still have a lot to learn about Orange.

[ajdapretnar]

We really appreciate it!

[smiks]

Looks ok. I have also tested it locally with different datasets (iris and my datasets). Tested different settings to see if both parameters work. Looks like it works, otherwise I wouldn't get this message Maximum number of iteration reached before convergence. Consider increasing max_iter to improve the fit.

[smiks]

Thanks everyone. I'm happy to have contributed to Orange.