Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ROC and confusion matrix to pipeline plot utils (#696) #704

merged 2 commits into from Apr 24, 2020


Copy link

@dsherry dsherry commented Apr 23, 2020

Preface: this is a clone of PR #696 -- I merged that to master this morning, but ran into RTD timeouts (described in #702). Reset master and now trying again.

Fix #639 #365 #392 #427 #608 #610 #620

Prior to #346 , plot data functions like ROC and confusion matrix were treated as objectives. #346 and follow-on work separated them from objectives, and made it so that these plot data functions aren't precomputed during automl. We then filed epic #639 to figure out where to put the code for plot data functions like ROC and confusion matrix.

This PR has the following changes:

  • Move ROC and confusion matrix functions to evalml/pipelines/ as standalone functions which take actual vs predicted
  • Delete plot_metrics input from automl, and delete plot_data field from automl results, because those were made unnecessary when we took ROC/confusion out of automl
  • Added a couple more gen_utils methods to the API docs (like import_or_raise)
  • Delete MSLE objective for codecov -- it was disabled with a comment
  • Add a couple misc tests to satisfy codecov.

Future work:

Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #704 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #704      +/-   ##
+ Coverage   99.09%   99.19%   +0.10%     
  Files         139      140       +1     
  Lines        4954     4952       -2     
+ Hits         4909     4912       +3     
+ Misses         45       40       -5     
Impacted Files Coverage Δ
evalml/objectives/ 100.00% <ø> (ø)
evalml/objectives/ 100.00% <ø> (+0.43%) ⬆️
evalml/objectives/ 100.00% <ø> (+5.55%) ⬆️
evalml/tests/utils_tests/ 100.00% <ø> (ø)
evalml/utils/ 100.00% <ø> (+1.69%) ⬆️
evalml/automl/ 97.05% <100.00%> (+0.63%) ⬆️
evalml/automl/ 100.00% <100.00%> (ø)
evalml/pipelines/ 100.00% <100.00%> (ø)
evalml/pipelines/ 100.00% <100.00%> (ø)
evalml/tests/automl_tests/ 100.00% <100.00%> (ø)
... and 7 more

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 fe7e929...87f2f2e. Read the comment docs.

@dsherry dsherry marked this pull request as ready for review Apr 23, 2020
Copy link
Collaborator Author

dsherry commented Apr 23, 2020

All checkin tests are green. The RTD build is green.

Because of #702, before I merge this, I will try to build this locally using the RTD docker container. I'll also rekick the job on RTD a couple times. I'm not sure what else to try!

Copy link
Collaborator Author

dsherry commented Apr 23, 2020

I rekicked the RTD job for this branch again:

I also rekicked latest:

All passed with flying colors.

I also followed the instructions on #586 to use the RTD docker container to build the docs locally. That succeeded without any issues. I was able to view the fresh html files and they looked fine.

dsherry added 2 commits Apr 24, 2020
* Impl

* Rename file

* Update test

* Forgot to add the new file

* Fix imports and lint

* Fix test

* Changelog

* Fix api docs

* Add missing markdown descr. Delete some ipynb elements which were failing validation

* Update docstrings

* Delete MSLE for codecov

* Increase test coverage for codecov

* Disallow unsupported options for normalize_confusion_matrix

* Add test coverage for get_objective w\ nonetype (for codecov)

* Update docstring

* Remove unnecessary raise in test mock

* Update test.

* Update docstring
Copy link

@angela97lin angela97lin left a comment

Hope it works this time!

@dsherry dsherry force-pushed the ds_639_clean_up_roc_confusion_take2 branch from 9a64d36 to 87f2f2e Compare Apr 24, 2020
Copy link
Collaborator Author

dsherry commented Apr 24, 2020

I hope so too!

I just rebased. I'll watch RTD pass once more, and then merge.

Copy link
Collaborator Author

dsherry commented Apr 24, 2020

The RTD build passed and the docs look good. Merging.

@dsherry dsherry merged commit b12e5fa into master Apr 24, 2020
2 checks passed
@dsherry dsherry deleted the ds_639_clean_up_roc_confusion_take2 branch Apr 24, 2020
Copy link
Collaborator Author

dsherry commented Apr 24, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

Pipeline plot data
2 participants