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

Overlapping plots #99

Closed
idomic opened this issue Dec 21, 2022 · 7 comments
Closed

Overlapping plots #99

idomic opened this issue Dec 21, 2022 · 7 comments
Assignees

Comments

@idomic
Copy link
Contributor

idomic commented Dec 21, 2022

When trying to print multiple plots via the same cell, the matrix and feature importance are overlapping.

i.e:

KNN = KNeighborsClassifier()
KNN.fit(X_train, y_train)
predictions= KNN.predict(X_test)
print(accuracy_score(y_test, predictions))
print(classification_report(y_test, predictions))
knn_cm = plot.confusion_matrix(y_test, predictions)
plot.feature_importances(rfc, top_n=5)

Screen Shot 2022-12-21 at 3 24 33 PM

@edublancas
Copy link
Contributor

Ah, this problem goes back a while due to an error I made when creating the first plots.

Our plotting methods have a ax argument that allows you to use a matplotlib.Axes object and add your plot there. The typical use case is someone plotting 4 plots in a 2 x 2 grid, and then passing the Axes objects to the plotting methods to fill the grid.

However, when a user does not pass the ax argument, we set a default, currently, we're setting it as:

ax = plt.gca()

This means "get current axes". The problem with this approach is that it'll re-use the same axes (that's why the plots overlap). I'm unsure what's matplotlib's rule for deciding when to create a new Axes object. Maybe if running it in a different cell will force creating a new axes?

I think what's best is to do:

_, ax = plt.subplots()

just like sklearn does

This will create a new axes object. I don't think this will have any impact for end-users, but once we make the change, we can check the rendered docs and examples to see if anything looks odd.

@edublancas
Copy link
Contributor

found another instance of overlapping plots:

image

https://sklearn-evaluation.ploomber.io/en/latest/api/plot.html#confusionmatrix

@edublancas
Copy link
Contributor

note: I fixed this for confusion matrix, this PR can be used as reference: #208

@WSShawn
Copy link
Contributor

WSShawn commented Jan 13, 2023

okay! I'll check it out!

@edublancas
Copy link
Contributor

this has been fixed #211

@idomic
Copy link
Contributor Author

idomic commented Jan 29, 2023

Is there anything else or we cna close this?

@edublancas
Copy link
Contributor

nothing else to do, we can close it

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

No branches or pull requests

3 participants