Skip to content
This repository was archived by the owner on Feb 28, 2025. It is now read-only.

fixed error in roc.py causing blank plots #250

Merged
merged 13 commits into from
Jan 27, 2023

Conversation

gtduncan
Copy link
Contributor

@gtduncan gtduncan commented Jan 26, 2023

Describe your changes

Found error in roc.py in which a call to plot() was missing a parameter. ROC curves should now be able to be graphed on subplots instead of separately from a blank plot, seen here

Issue ticket number and link

Closes #231

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added thorough tests (when necessary).
  • I have added the right documentation in the docstring and changelog (when needed)

📚 Documentation preview 📚: https://sklearn-evaluation--250.org.readthedocs.build/en/250/

@gtduncan gtduncan changed the title fixed error in roc.py fixed error in roc.py causing blank plots Jan 26, 2023
@coveralls
Copy link

coveralls commented Jan 26, 2023

Pull Request Test Coverage Report for Build 4028181701

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.172%

Totals Coverage Status
Change from base Build 4028106220: 0.0%
Covered Lines: 2650
Relevant Lines: 2814

💛 - Coveralls

gtduncan and others added 6 commits January 26, 2023 16:24
rerunning docs

docs rerun
* test

* pinning scikit-learn

* replaces papermill with ploomber-engine

docs update

docs update
@gtduncan gtduncan marked this pull request as ready for review January 27, 2023 14:08
Copy link
Contributor

@idomic idomic left a comment

Choose a reason for hiding this comment

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

Besides the changelog entry looks good.

CHANGELOG.md Outdated
@@ -6,6 +6,7 @@
- [API Change] A new figure and axes is created (via `plt.subplots()`) when calling a plotting method with `ax=None`. Previously, the current axes was used (via `plt.gca()`) ([#211](https://github.com/ploomber/sklearn-evaluation/pull/211))
- [Fix] Validating input elbow curve model has "score" method [#146]
- [Fix] Adds class labels for multi class roc plot (#209)
- [Fix] Fixed error in which ROC curves would not graph on subplots
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be [Docs], the text should probably be, Fixed ROC curves plots to show properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting one because the flaw was discovered in the docs, but the actual fix is in the source code. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, maybe we can call it a fix after all. @gtduncan

Copy link
Contributor

Choose a reason for hiding this comment

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

So if it fits both categories, we should probably take the code first approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something weird is happening here as well, the tests are running for 3 hours already.

Copy link
Contributor Author

@gtduncan gtduncan Jan 27, 2023

Choose a reason for hiding this comment

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

That's what I was about to ask about, but it seems to be happening to the CI checks for all the PRs today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran all the tests locally and they passed

Copy link
Contributor

Choose a reason for hiding this comment

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

Stay tuned, work on other issues :)

Copy link
Contributor

Choose a reason for hiding this comment

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

the error has been fixed, please rebase

gtduncan and others added 3 commits January 27, 2023 11:08
Updating docs dependencies to ploomber-engine (ploomber#252)

* test

* pinning scikit-learn

* replaces papermill with ploomber-engine

merge

changelog corrected
@idomic idomic merged commit 662368e into ploomber:master Jan 27, 2023
@idomic
Copy link
Contributor

idomic commented Jan 27, 2023

Nice job @gtduncan !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blank plots in matplotlib guide
4 participants