Making the sklearn estimator compliant

natekupp / ffx

Fast Function Extraction

http://trent.st/ffx

Other

80 stars 93 forks source link

Making the sklearn estimator compliant #45

Closed pizzooid closed 3 years ago

pizzooid commented 4 years ago

Replaces #36

coveralls commented 4 years ago

Coverage increased (+0.6%) to 77.041% when pulling f68cb76702079b5500426b8debc9bc4fa6d8054c on pizzooid:check_estimator into 0e1552ab1be4bcba0bcc38ba74ee4a8fced75959 on natekupp:master.

pizzooid commented 4 years ago

What should we do about this? The job exceeded the maximum log length, and has been terminated.

natekupp commented 4 years ago

Hey @pizzooid thanks for adding this, and apologies for the delayed response!

Looks due to:

ConvergenceWarning: Objective did not converge. You might want to increase the number of iterations. Duality gap: 0.015931260617070172, tolerance: 0.004999999999999998

maybe we need to increase # of iterations to make this work? If I get some time in the next couple of weeks I can investigate further

pizzooid commented 4 years ago

No worries. ;-)

I don't think that increasing the iterations number is going to get rid of all warnings. I think that the elastic net converges too slowly for some problems. The tests just uncover this problem. :-(

Maybe by looking at the inputs it is possible to see for which input values these errors are occuring.

What do you think?

natekupp commented 3 years ago

@maxiimilian @pizzooid sure, could one of you rebase on master just to confirm this will merge cleanly? and ideally we can get the tests to pass for this PR before landing it

natekupp commented 3 years ago

closing this in favor of #50