DistrictDataLabs / yellowbrick

Visual analysis and diagnostic tools to facilitate machine learning model selection.
http://www.scikit-yb.org/
Apache License 2.0
4.3k stars 559 forks source link

Fix KElbow get_params #1251

Closed bbengfort closed 2 years ago

bbengfort commented 2 years ago

This PR fixes #1232 which reported a bug that caused a problem to occur when users call KElbow.get_params(), e.g. in a notebook when they want to render the scikit-learn rich visualization of the estimator.

I have made the following changes:

  1. Updated KElbow to ensure it stores all hyperparams and only creates learned params in fit()
  2. Added tests to ensure get_params works for KElbow
  3. Updated the wrapper to prevent infinite recursion and give a more meaningful attribute error for future debugging

Sample Code and Plot

from sklearn.cluster import KMeans
from yellowbrick.cluster import KElbowVisualizer
visualizer = KElbowVisualizer(KMeans(), k=(2,12))
visualizer.get_params()

The above code snippet should not produce an exception: AttributeError: 'KMeans' object has no attribute 'k'

Questions for the @DistrictDataLabs/team-oz-maintainers:

CHECKLIST

codecov[bot] commented 2 years ago

Codecov Report

Merging #1251 (941e793) into develop (233d9d1) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1251   +/-   ##
========================================
  Coverage    90.57%   90.58%           
========================================
  Files           92       92           
  Lines         5209     5213    +4     
========================================
+ Hits          4718     4722    +4     
  Misses         491      491           
Impacted Files Coverage Δ
yellowbrick/model_selection/dropping_curve.py 92.18% <ø> (ø)
yellowbrick/cluster/elbow.py 97.81% <100.00%> (-0.04%) :arrow_down:
yellowbrick/utils/wrapper.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 233d9d1...941e793. Read the comment docs.

lwgray commented 2 years ago

It was dropping curve...

lwgray commented 2 years ago

@bbengfort You should open an issue to create get_params test.

bbengfort commented 2 years ago

I just checked DroppingCurve - get_params works fine, so I guess you fixed it.

bbengfort commented 2 years ago

@lwgray Issue #1252 opened at your request.

bbengfort commented 2 years ago

I also added a test for DroppingCurve.get_params.