-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[ENH] Scatter Plot: Add ellipse/s #6752
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6752 +/- ##
==========================================
+ Coverage 88.13% 88.18% +0.05%
==========================================
Files 322 326 +4
Lines 70589 70994 +405
==========================================
+ Hits 62216 62609 +393
- Misses 8373 8385 +12 |
Screen.Recording.2024-03-25.at.13.22.56.mov |
Previously, the stretch of the ellipse was |
Lines can also minimize perpendicular distance if you set the plot to treat variables as independent. |
By using the option "treat variables as independent" the regression line actually lines up with the ellipse. Do we want to prioritize this? |
By making it a default? Probably not because users expect a regression line. Yet, scatter plot shows relation between two variables, so perpendicular projection should be the default. We never found a good solution to make users aware that there are two possibilities and that they must decide what they want. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the fix everything looks ok.
As discussed we should maybe move this to the base class.
# https://stats.stackexchange.com/questions/577628/trying-to-understand-how-to-calculate-a-hotellings-t2-confidence-ellipse | ||
# https://stackoverflow.com/questions/66179256/how-to-check-if-a-point-is-in-an-ellipse-in-python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe replace with the following reference for HotellingEllipse, from where we took the equation to scale for 95%
https://github.com/ChristianGoueguel/HotellingEllipse/blob/master/R/ellipseCoord.R
cx = np.cos(m) * np.sqrt(cov[0, 0] * f) | ||
cy = np.sin(m) * np.sqrt(cov[1, 1] * f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use @thocevar 's fix.
Issue
Implements #6734
Description of changes
Add ellipse (similar to regression line).
Includes