Skip to content

DOC improved example plot in plot_lda_qda.py #12942

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

Merged
merged 9 commits into from
Jan 14, 2019

Conversation

zyxue
Copy link
Contributor

@zyxue zyxue commented Jan 8, 2019

Improved the plots on https://scikit-learn.org/stable/auto_examples/classification/plot_lda_qda.html#sphx-glr-auto-examples-classification-plot-lda-qda-py.

Currently, the quadratic decision boundary of QDA is hardly seen in the 4th subplot. As the boundary being quadratic is the most important distinguishment from that of LDA, the current version could be confusing.

The updated plot:

Imgur

@jnothman
Copy link
Member

jnothman commented Jan 8, 2019

Thanks for the pr. In the image above, the two lower figures should look like identical datasets, but now don't

@zyxue
Copy link
Contributor Author

zyxue commented Jan 8, 2019

they're the same, how not? It's just that misclassified data are marked with crosses.

I haven't modified data generation process at all.

@jnothman
Copy link
Member

jnothman commented Jan 9, 2019

Ah, you're right. I was confused visually by the ellipses which are too close in colour to the points

@jnothman
Copy link
Member

jnothman commented Jan 9, 2019 via email

@zyxue
Copy link
Contributor Author

zyxue commented Jan 9, 2019

Ellipses are colored this to be consistent with what data class they're representing. Increasing alpha will obscure the data points even more, and I am not sure what you mean by contrast here.

Honestly, I think this is good, much clearer compared to the original plot. The confusion may be because there is a lot of information to show on this plot, it may take a moment to figure out which is which.

@jnothman
Copy link
Member

jnothman commented Jan 9, 2019 via email

@zyxue
Copy link
Contributor Author

zyxue commented Jan 9, 2019

alpha to matplotlib.patches.Eclipse has no effect although it's documented, could be a bug.

I could make it transparent, as seen here. Let me know if it's ok. If so, I will push the update.

@jnothman
Copy link
Member

jnothman commented Jan 9, 2019 via email

@jnothman jnothman changed the title improved example plot in plot_lda_qda.py DOC improved example plot in plot_lda_qda.py Jan 9, 2019
@zyxue
Copy link
Contributor Author

zyxue commented Jan 9, 2019

Should be good now. The current render:
Imgur

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Happy for the suptitle to go.
LGTM. Let's see whether other core devs have opinions.

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

thanks @zyxue I think your version is much better.

@qinhanmin2014 qinhanmin2014 merged commit ba4720a into scikit-learn:master Jan 14, 2019
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Feb 19, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
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.

3 participants