Add new example for plotting a confidence_ellipse #13570
Conversation
| kwargs : `~matplotlib.patches.Patch` properties | ||
|
|
||
| author : Carsten Schelp | ||
| license: GNU General Public License v3.0 (https://github.com/CarstenSchelp/CarstenSchelp.github.io/blob/master/LICENSE) |
There was a problem hiding this comment.
Not a lawyer so pinging @tacaswell on this, but I think GPL won't be acceptable. Anyone can dismiss once resolved.
There was a problem hiding this comment.
And most other examples don’t have an author block....
There was a problem hiding this comment.
Not a lawyer so pinging @tacaswell on this, but I think GPL won't be acceptable. Anyone can dismiss once resolved.
When no other example bothers with licenses I won't, either. I just thought that the python license was GPLv3.0 compatible?
There was a problem hiding this comment.
Oh - matplotlib has a license of its own. Either way - I removed the license block. I hope that it is ok now if I resolve this conversation?
There was a problem hiding this comment.
@anntzer: Hi, thanks to the community this is pull request is approaching a state that might be called 'mature'. Do you agree that your change request has been addressed appropriately? It was about that probably problematic license-tag in the docstring.
|
Thanks for the PR! There are a few styling issues; we don't apply PEP8 extremely strictly on the examples but I'd suggest you run flake8 on the file and fix as many of the style points it raises. |
|
Thank you for the reviews! I have applied the suggested changes. |
|
This is technically ok, but it's a lot of code (see current doc build for the rendered result). Putting all the code into a single code block might be a bit scary to the reader. Have you considered splitting the example into multiple sections? See e.g. https://matplotlib.org/devdocs/gallery/lines_bars_and_markers/joinstyle.html (source is in examples/lines_bars_and_markers/joinstyle.py). |
|
@timhoffm Hi Tim, you are right - it even scared me! Thanks for pointing me to that example. The one I picked to get started did not have to use more than one code block. I will be back with a more friendly layout shortly. |
|
I hope that I managed to make the example more user-friendly, now. |
|
Ellipse takes x,y width height and angle, so why do you use transforms to supply those parameters? |
Hi @jklymak, The effect is that the same transform would be applied a code-layer lower. As for the translation transform - it cannot be worked away by specifying (mean_x, mean_y) instead of the origin (0, 0) as the center of the ellipse. The scale transform is relative to the origin and the ellipse shifts to the wrong place when it is scaled while it is already in its final place. Some implementations of scale() allow the user to specify a center of the scaling transform other than the origin. But this one does not. (Also this would mean nothing else than shifting the ellipse back and forth once more.) Executing the transforms explicitly does not degrade efficiency as far as I can tell. Also I like to see all transforms in the same code-layer rather than having some executed here and some there. |
|
Aah! One wrong click! Sorry for this closing-reopening hubbub. Went by accident. |
… plots are closely related and thus go into one figure.
Yes, but the naive user's tendency would be to not apply a transform. If you need one then great, but explain why. I'm not convinced you do, but I could certainly be wrong...
I don't understand why you need a scale transform at all.
Do you link the article? Because I'm not following why you don't know the width and height a-priori. If you can't do this any other way than by specifying transforms it would be good to explain it clearly to the user, because otherwise they will wonder why you aren't just rotating at the start. |
The link to the article is in the docstring of the method. But in fact, the approach does need some explanation and maybe the link should be in a more prominent place. Like the first heading that goes "The plotting function itself ...". |
|
First, I agree that an example like this is useful. Second, I understand what you are doing, and I understand why you want to do it this way, but I feel it is a misuse of the transform stack. Instead I think you should consider getting the eigenvectors and eigenvalues and using those to calculate the angle, width, and height as outlined in the first answer to this stack overflow question: https://stackoverflow.com/questions/12301071/multidimensional-confidence-intervals/12321306#12321306 |
|
@jklymak: Thank you for really diving into this, Jody. |
TODO: pass facecolor parameter to Ellipse() explicitly. Co-Authored-By: CarstenSchelp <carstenschelp@mp.nl>
…s '.T' Co-Authored-By: CarstenSchelp <carstenschelp@mp.nl>
Co-Authored-By: CarstenSchelp <carstenschelp@mp.nl>
…t value for it in confidence_ellipse().
…/matplotlib into confidence_ellipse
|
I haven't looked too much into it, but after a cursory look I think I do like the approach of using transforms to achieve the desired result. |
|
If there is a controversity about the method being used, maybe one can add another function showing the eigenvector decomposition at the bottom of the example and show how both methods would yield the same result? |
| # Different number of standard deviations | ||
| # """"""""""""""""""""""""""""""""""""""" | ||
| # | ||
| # A plot with n_std = 3 (gray), 2 (blue) and 1 (red) |
There was a problem hiding this comment.
Could be some thing more descriptive like "A plot with multiple ellipses describing different standard deviations". The exact numbers and colors are explained in the legend and don't need to be described here.
There was a problem hiding this comment.
Oh, I didn't see this one. I will take a look shortly.
| [-0.2, 0.35] | ||
| ]) | ||
| mu = np.array([0, 0]).T | ||
| scale = np.array([8, 5]).T |
There was a problem hiding this comment.
Be consistent in the way you define mu and scale (see above).
There was a problem hiding this comment.
If there is a controversity about the method being used, maybe one can add another function showing the eigenvector decomposition at the bottom of the example and show how both methods would yield the same result?
So far all comments and suggestions were aiming for an example as concise and readable as possible. This is a good thing and I would not add anything extra.
The core of this example is to show a particular way to get that ellipse and to explain why this works (via a link, that is). There are other ways to get that ellipse and the linked article links to one, in its turn, too. If somebody wants to show another way that should happen in another example, I think. Should be a quick job with some - perfectly legitimate - copy and paste.
|
For easier review: Current doc build |
Remove another facecolot='none' that apparently slipped through. Co-Authored-By: CarstenSchelp <carstenschelp@mp.nl>
Remove superfluous string interpolation-'f' that slipped through. Co-Authored-By: CarstenSchelp <carstenschelp@mp.nl>
…/matplotlib into confidence_ellipse
|
Likewise, Tim, and everyone else! Nice too see that people find it worthwhile! |
|
Small markup improvements remain possible, but this is already better than 99% of the examples, so I'm not going to nitpick :) merging. |
…570-on-v3.1.x Backport PR #13570 on branch v3.1.x (Add new example for plotting a confidence_ellipse)
PR Summary
New statistics-example with a robust and exact way to plot a confidence ellipse of a two-dimensional dataset.
PR Checklist