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

fix overlapping plots #211

Merged
merged 4 commits into from
Jan 17, 2023
Merged

Conversation

WSShawn
Copy link
Contributor

@WSShawn WSShawn commented Jan 14, 2023

Describe your changes

fixed overlapping plots

Issue ticket number and link

Closes #x

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 (when needed). Product update? If yes, write one line about this update.

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

@coveralls
Copy link

coveralls commented Jan 14, 2023

Pull Request Test Coverage Report for Build 3917033782

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 14 of 16 (87.5%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 94.622%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/sklearn_evaluation/plot/precision_recall.py 1 3 33.33%
Totals Coverage Status
Change from base Build 3915211639: 0.2%
Covered Lines: 2639
Relevant Lines: 2789

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3917033782

  • 14 of 16 (87.5%) changed or added relevant lines in 11 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 94.55%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/sklearn_evaluation/plot/precision_recall.py 1 3 33.33%
Files with Coverage Reduction New Missed Lines %
src/sklearn_evaluation/telemetry.py 2 96.92%
Totals Coverage Status
Change from base Build 3915211639: 0.1%
Covered Lines: 2637
Relevant Lines: 2789

💛 - Coveralls

@idomic
Copy link
Contributor

idomic commented Jan 14, 2023

I see no tests, please check this pr, it has sample confusion matrix plot tests, add the necessary tests for each plot.

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 14, 2023

Hey so I was reading through the PR you mentioned. I have some questions regarding how to deploy the test "for each plot" and where am I supposed to find the correct baseline images. Would be super helpful if there could be a tutorial session for this. Thank you.

@edublancas
Copy link
Contributor

in my sample confusion matrix test, I'm generating multiple plots (e.g., cm1 + cm2), so it's easy to test the subplot, because I just check that many image files are generated. For cases where we're testing single images, I think it'll be better to mock plt, and then test that subplots has been called - that would be enough to test we won't have overlapping plots

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 16, 2023

sanity check: difference in feature importances plots after using subplots (different features included on the plot). slight differences in most confusion matrix plots by a value of 1 in some class. slight differences in the classification report plots (0.98 change to 1.0).

@idomic
Copy link
Contributor

idomic commented Jan 16, 2023

A change in the values inside is fine, the main thing to check is that there's no weird stuff with the plots: missing labels, overlapping text, etc.

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 16, 2023

Yeah I did not see any of those problems!

@edublancas
Copy link
Contributor

please also share screenshots so we can better judge the differences

@edublancas
Copy link
Contributor

also, let's change the changelog and tag this as [API Change], as it impacts the API

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 16, 2023

Sure!

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 16, 2023

Screenshot 2023-01-16 at 11 32 37 AM
![Screenshot 2023-01-16 at 11 32 58 AM](https://user-images.githubusercontent.com/52613022/212727312-e8d217
Screenshot 2023-01-16 at 11 33 17 AM
0f-ac1e-48a0-8547-0f205bda15c8.png)

On the left is original and on the right is ones that use subplots

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 16, 2023

should we modify the changelog for 0.9.0 or 0.9.1?
Screenshot 2023-01-16 at 11 37 25 AM

@idomic
Copy link
Contributor

idomic commented Jan 16, 2023

Always the dev version.

@edublancas
Copy link
Contributor

change 0.9.1.dev -> 0.10dev and also update the value in the src/sklearn_evaluation/init.py to 0.10dev, then put your changelog entry under 0.10dev

@WSShawn
Copy link
Contributor Author

WSShawn commented Jan 16, 2023

got it. just to make sure, it will look like this:

CHANGELOG

0.10.0dev
BP [API Change] Fixed overlapping plot issue (#211)

0.9.0 (2023-01-13)
...

@edublancas edublancas merged commit 94c6f61 into ploomber:master Jan 17, 2023
neelasha23 pushed a commit to neelasha23/sklearn-evaluation that referenced this pull request Jan 18, 2023
* fix overlapping plots

* update changelog and src/init.py

0.9.1dev to 0.10dev

* fix init.py typo

* Update CHANGELOG.md

Co-authored-by: Eduardo Blancas <[email protected]>

Pr curve class added

Testcases

tests

Fixed duplicate labels

More tests

Added docs

test image

lints

removed numpy

Added tol

Lint

modify exceptions tests

Lint

Review

Review comments

Lint

Added tol

ploombererror
@WSShawn WSShawn deleted the fix_overlapping_plots branch January 19, 2023 17:55
@edublancas edublancas mentioned this pull request Jan 29, 2023
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.

4 participants