Skip to content

Increase coverage of model understanding functions in docs#3446

Merged
eccabay merged 12 commits intomainfrom
3524_mu_docs_coverage
Apr 11, 2022
Merged

Increase coverage of model understanding functions in docs#3446
eccabay merged 12 commits intomainfrom
3524_mu_docs_coverage

Conversation

@eccabay
Copy link
Contributor

@eccabay eccabay commented Apr 5, 2022

We were missing descriptions of a few functions, this adds those.

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #3446 (3cc9420) into main (4585cd1) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main   #3446   +/-   ##
=====================================
  Coverage   99.7%   99.7%           
=====================================
  Files        336     336           
  Lines      33297   33297           
=====================================
  Hits       33165   33165           
  Misses       132     132           

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 4585cd1...3cc9420. Read the comment docs.

@eccabay eccabay marked this pull request as ready for review April 5, 2022 19:00
Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @eccabay !

Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really love this, thank you.

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work adding these in! There is a graphing issue that I think blocks this, but left some nits as well!

" })\n",
"pipeline_linear.fit(X_train, y_train)\n",
"\n",
"get_linear_coefficients(pipeline_linear.estimator, features=X.columns)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Not sure if this would be good as a change to the function output or just a small change to the cell, but I think it'd look cleaner and make more sense for OS users if we showed this as a dataframe with column names labeled:
image

Similar to
image

"source": [
"from evalml.model_understanding import graph_t_sne\n",
"\n",
"graph_t_sne(X)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no output here:
image

Filed an issue here but likely need to address that first before we can move in with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So weird and good catch @bchen1116 ! Maybe we can try to build the docs locally to see if they show up in the generated html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bchen1116 since this graphing issue is independent of these changes, not sure it's worth it to block this PR on them getting fixed

@eccabay eccabay requested a review from bchen1116 April 8, 2022 18:09
Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @eccabay! The PR looks good to me! Hope we can get graphing fixed in the docs soon, but let's get this in first!

@eccabay eccabay merged commit 7abdc7d into main Apr 11, 2022
@eccabay eccabay deleted the 3524_mu_docs_coverage branch April 11, 2022 14:46
@chukarsten chukarsten mentioned this pull request Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants