Fixes to statistical recipes #1563

piever · 2022-01-09T11:36:00Z

Description

Fixes qqnorm, qqplot, which were broken as they were converted to an odd Scatter plot type and that broke with the new internals. They now have a more sensible "standard" implementation as a new plot type.

It also fixed plotting StatsBase.Histogram objects. They accidentally ended up having a gap between bars after ,#1223.

Type of change

Delete options that do not apply:

Bug fix (non-breaking change which fixes an issue)

Checklist

Added or changed relevant sections in the documentation

I have not added tests as it seems tests exist already (see https://github.com/JuliaPlots/Makie.jl/blob/master/test/statistical_tests.jl) but somehow they stopped being run. It is probably best to update those in a separate PR (it seems all of them need fixing, due to changes in the internals).

SimonDanisch

Not really the right person to review this ;) If you have opinions on this @jkrumbiegel, please comment as well..Otherwise, I'd just leave this up to @piever to merge!

jkrumbiegel · 2022-01-10T14:53:03Z

src/stats/distributions.jl

+- a list of samples,
+- an abstract distribution, e.g. `Normal(0, 1)`,
+- a distribution type, e.g. `Normal`.
+In the last case, the Q-Q plot by fitting that distribution type to the data `y`.


This sentence misses a word?

jkrumbiegel · 2022-01-10T14:55:18Z

src/stats/distributions.jl

+The attribute `qqline` (defaults to `:R`) determines how to compute a fit line for the Q-Q plot.
+Possible values are the following.
+- `:identity` draws the identity line (useful to see `x` and `y` follow the same distribution).
+- `:fit` fits the line to the quantile pairs (useful to see if the distribution of `y` can be obtained from the distribution of `x` via an affine transformation).


Maybe :fit and :quantile are not the best names. Both fit and both use quantiles it seems to me?

Yes, I'm renaming :quantile to :robust (that's how Base R calls it, see here under qq.line.type). They say "least squares" instead of :fit, but I think that's a bit too technical.

jkrumbiegel · 2022-01-10T14:56:48Z

src/stats/distributions.jl

+- `:identity` draws the identity line (useful to see `x` and `y` follow the same distribution).
+- `:fit` fits the line to the quantile pairs (useful to see if the distribution of `y` can be obtained from the distribution of `x` via an affine transformation).
+- `:quantile` is analogous to `:fit` but uses a quantile-based fitting method.
+- `:R` is an alias for `:quantile`, as that is the default behavior in `:R`.


Hm I'm not sure if that's necessary to have two names for the same thing where one is R. We don't do that in other places either, so maybe a comment that quantile is Rs default method is enough?

Actually, it looks like Base R (see here) and ggplot2 (see here) have different defaults, so I'm just omitting that part.

jkrumbiegel · 2022-01-10T14:57:21Z

src/stats/distributions.jl


+Graphical attributes are
+- `color` to control color of both line and markers


If markercolor is not also specified I guess?

jkrumbiegel · 2022-01-10T14:59:03Z

src/stats/distributions.jl


-convert_arguments(::Type{<:QQPlot}, args...; kwargs...) =
-    convert_arguments(Scatter, qqbuild(loc(args...)...); kwargs...)
+Shorthand for `qqplot(Normal, y)`. See [`qqplot`](@ref) for more details.


I think this @ref stuff hasn't worked for a while, maybe since the switch to Franklin? At least I remember having to remove it in a bunch of places

piever · 2022-01-10T16:23:54Z

Thanks for the review! I've addressed the feedback and added a NEWS entry on the renaming of the options for qqline . Should be good to go now.

piever · 2022-01-10T16:34:17Z

Actually, I do have one doubt regarding the default value for qqline. I realize that no single choice is ideal, because qqline=:robust makes a lot of sense if you are doing the standard qqnorm (comparing with a standard Gaussian, or more generally a distribution with default parameters), whereas qqline=:identity is IMO a more reasonable default if you are comparing two empirical distributions (from vectors of samples) or data versus a fitted distribution.

OTOH, I don' really like magically switching from one default to the other depending on the argument types. Should we just default to qqline=:none, or is qqline=:robust a good enough default?

UPDATE: I'm becoming convinced that, for the sake of clarity, one should do the following two things.

Default to qqline=:none, otherwise the behavior (figuring out when it's a fit and when it's the identity line) is too subtle.
Rename :robust (the old :quantile) to :fitrobust. It is conceptually not different from :fit, but it simply uses only the [0.25, 0.75] quantiles rather than the range(0, 1, length=N) (which is used to draw the plot).
Error for invalid values of qqline and list the valid options in the error message.

(I've updated the PR to reflect this approach.)

palday · 2022-01-15T04:03:54Z

@piever I like the idea of omitting the line by default and forcing the user to take an explicit stand on which type they want.

src/stats/distributions.jl

Co-authored-by: Phillip Alday <palday@users.noreply.github.com>

piever · 2022-01-21T09:55:03Z

Planning to merge this in a few days if there are no additional concerns. To summarize the main API changes

old qqline=:quantile or qqline=:R are now qqline=:fitrobust
:none is the default (as in general it is unclear exactly which line should be drawn)
a value of qqline not among the valid options (:none, :fit, :fitrobust, :identity) errors rather than being ignored silently (which allows us to add options in a non-breaking way in the future)

piever added 5 commits January 9, 2022 12:04

fix hist and qqplot

8947bc4

add qqplot docs

b7481be

add docs page

85ff5a4

revert default to R

08ca987

include qqnorm docstring

48d0230

piever mentioned this pull request Jan 9, 2022

bump makie version MakieOrg/AlgebraOfGraphics.jl#323

Merged

SimonDanisch approved these changes Jan 9, 2022

View reviewed changes

SimonDanisch requested a review from jkrumbiegel January 9, 2022 14:45

jkrumbiegel reviewed Jan 10, 2022

View reviewed changes

piever added 3 commits January 10, 2022 17:05

address review comments

b061436

mention default value of attribute in news

420c01c

clean fitting code

d6b10ed

piever added 2 commits January 14, 2022 17:15

fix numerical imprecision in qqline computation

a501b6f

update news entry

cfebfe7

palday reviewed Jan 15, 2022

View reviewed changes

src/stats/distributions.jl Outdated Show resolved Hide resolved

palday mentioned this pull request Jan 15, 2022

Compat fixes for Makie 0.16 palday/MixedModelsMakie.jl#47

Merged

Pietro Vertechi and others added 4 commits January 15, 2022 15:30

Update src/stats/distributions.jl

069d01b

Co-authored-by: Phillip Alday <palday@users.noreply.github.com>

remove unneeded extra method

a47f7d3

default to none for qqline

bc93752

update news

de33041

piever force-pushed the pv/qqplot branch from 1cf3ee2 to de33041 Compare January 16, 2022 17:43

fix error message on older julia

bce0d05

piever merged commit 584271a into master Jan 25, 2022

piever deleted the pv/qqplot branch January 25, 2022 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to statistical recipes #1563

Fixes to statistical recipes #1563

piever commented Jan 9, 2022

SimonDanisch left a comment

jkrumbiegel Jan 10, 2022

jkrumbiegel Jan 10, 2022

piever Jan 10, 2022 •

edited

Loading

jkrumbiegel Jan 10, 2022

piever Jan 10, 2022

jkrumbiegel Jan 10, 2022

jkrumbiegel Jan 10, 2022

piever commented Jan 10, 2022 •

edited

Loading

piever commented Jan 10, 2022 •

edited

Loading

palday commented Jan 15, 2022

piever commented Jan 21, 2022


		Graphical attributes are
		- `color` to control color of both line and markers

Fixes to statistical recipes #1563

Fixes to statistical recipes #1563

Conversation

piever commented Jan 9, 2022

Description

Type of change

Checklist

SimonDanisch left a comment

Choose a reason for hiding this comment

jkrumbiegel Jan 10, 2022

Choose a reason for hiding this comment

jkrumbiegel Jan 10, 2022

Choose a reason for hiding this comment

piever Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

jkrumbiegel Jan 10, 2022

Choose a reason for hiding this comment

piever Jan 10, 2022

Choose a reason for hiding this comment

jkrumbiegel Jan 10, 2022

Choose a reason for hiding this comment

jkrumbiegel Jan 10, 2022

Choose a reason for hiding this comment

piever commented Jan 10, 2022 • edited Loading

piever commented Jan 10, 2022 • edited Loading

palday commented Jan 15, 2022

piever commented Jan 21, 2022

piever Jan 10, 2022 •

edited

Loading

piever commented Jan 10, 2022 •

edited

Loading

piever commented Jan 10, 2022 •

edited

Loading