[MRG] Attempt to make GLM compatible with scikit-learn check_estimator

[titipata]

So far, there are 3 main parts which can be fixed in GLM class to be compatible with scikit-learn's check_estimator

1. Initial attributes
2. fit_predict function
3. scipy's expit

closes #363

[jasmainak]

how are you testing? can you add the check_estimator line somewhere in the diff?

[titipata]

@jasmainak I did add a test script where you can check by running py.test --cov=pyglmnet tests/. Note that this will fail a bunch of original tests since I commented out fit_predict from GLM

[jasmainak]

Cool, FYI, I use this command:

$ pytest ./tests/test_pyglmnet.py::test_glm_estimator --pdb

that way you drop right into the console when there is an error :)

[jasmainak]

For whatever reason X is an array of complex numbers, that's why it is failing

[titipata]

@jasmainak Thanks for the handy test script. Oh yeah, I see that: z is ndarray of size (10, ) with a complex number in there...

[jasmainak]

I don't think it's easy to make all the checks happy without adding an sklearn dependency. Basically you have to follow what is in: https://github.com/scikit-learn-contrib/project-template/blob/master/skltemplate/_template.py. I'd like to believe it can be done though :)

[titipata]

@jasmainak alright, I tried adding sklearn dependency so that it follows their template style. Not sure if we also want BaseEstimator to be from sklearn also?

[jasmainak]

Let's try to first make it work. Use sklearn or whatever is necessary. Then we can make it nice and see if the dependency can be dropped easily :)

[titipata]

@jasmainak Now the error is in predict_proba, I'm not sure how to fix it.

[titipata]

@jasmainak alright the current commits should work with check_estimator now! The part that I cannot keep track is the _allow_refit. Does scikit-learn dependencies keep track of this fitting/re-fitting?

[jasmainak]

You can remove the refit logic for now. Do all tests pass if you do that? I’ll see what to do with the refit when I get a chance in a couple of hours

On Tue 18 Feb 2020 at 14:05, Titipat Achakulvisut ***@***.***> wrote: @jasmainak <https://github.com/jasmainak> alright the current commits should work with check_estimator now! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#364?email_source=notifications&email_token=ADY6FIX2I4DDDKREO2XCPL3RDQWODA5CNFSM4KXIRITKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMDSOHQ#issuecomment-587671326>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADY6FIRQK7MBYVQKRN4QW3TRDQWODANCNFSM4KXIRITA> .

-- Sent from my iPhone

[titipata]

@jasmainak There are 2 failed (test_random_state_consistency and test_api_input), 56 passed, 32 warning for the current commits. The error is related to Failed: DID NOT RAISE <class 'ValueError'>.

[titipata]

The test is now not raising errors because of the additional check_X_y. It returns nd.array back even you put the list in. So now, the function test_api_input in test_pyglmnet.py below won't raise an error:

    with pytest.raises(ValueError):
        glm.fit(X, list(y))

And the fail in test_random_state_consistency is coming from the commented out of self._allow_refit = False after fit method. Specifically, it comes from

    match = "This glm object has already been fit"
    with pytest.raises(ValueError, match=match):
        ypred_c = glm_b.fit_predict(Xtrain, ytrain)

[titipata]

Alright @jasmainak, I think this should be the push using sklearn dependency. If you can help a bit with _allow_refit logic, that should be it! I will leave it to you for now.

[jasmainak]

This is great!! awesome work @titipata :-) Happy to take over from here. Can you update whats_new.rst so we can acknowledge this in the next release?

[jasmainak]

@titipata you are on fire. I don't think I need to do a pass really. If you just address my last couple of comments (remove the commented test), move the line in whats_new.rst and update the title from WIP to MRG, I am happy to merge this PR once both CIs become green. Thank you so much for the efforts!

[jasmainak]

oops, the default value changed here.

[titipata]

Yes, somehow it fails sklearn with check_estimator due to predict_proba. I didn't resolve it here.

[jasmainak]

humm ... I see. Let's bring it back to the default and see if we can fix it? Is that the only thing left that didn't work?

[titipata]

Yes, will do bring it back to the default!

[titipata]

Awesome!! That sounds great to me! We might have to write some more test scripts later on :)

[jasmainak]

Yes I am looking forward to more contributions from you in the future!

[titipata]

@jasmainak, so I added the workaround for predict_proba error when using check_estimator. This is similar to the logic in sklearn's LogisticRegression.

[jasmainak]

Good to go from your end @titipata ? Everything resolved and works? Please set the PR title to MRG if so!

[titipata]

@jasmainak yes, I guess you can help polish a bit and that should be good to go!

[jasmainak]

I'll merge @titipata when the CIs are green. We're good to go!

[titipata]

Perfect 👍 @jasmainak. We can also poke JOSS editor right after. This PR should take care their concerns!

[jasmainak]

yep that's the plan! :)

[jasmainak]

All good, thanks a ton @titipata !