Move method descriptions to docstrings #106

BenjaminBorn · 2017-08-21T14:03:12Z

In this pull request, I have copied all available documentation on tests, pvalues, and confidence intervals to docstrings. Please note that I have only done some reformatting. I have not checked the documentation for correctness. This is the first step in the transition to Documenter.jl as discussed in #103.

When reviewing, please consider the following questions.

I have attached the general description of pvalue() and confint() somewhat arbitrarily to two of the tests (Binomial.jl and t.jl). Is there a better place to put them?
Sometimes a test has different methods, e.g. OneSampleTTest(). If possible, I have combined them in one docstring, but sometimes, for clarity reasons, I have included different docstrings for the methods. Is that ok, or should we combine them somehow into one docstring? The disadvantage of having them in different docstrings is that the help output in the REPL can get convoluted, especially as there is not much spacing between the output for the different methods.
pvalue and confint also have different methods with a number of separate docstrings. Is there a way to ensure that the general method, e.g. pvalue(x::HypothesisTests), is the first shown when typing ?pvalue? As some of the more specialised methods have long docstrings, it could get lost otherwise.
The admonition !!! note doesn't work in the Jupyter notebook (at least not for me using Safari). It works in the REPL and Juno. Maybe someone can try it. If it's a general problem, we can file an issue with IJulia (or at the appropriate place).
There was a copy/paste mistake in the docstring for the binomial confidence intervals. After fixing it, I realised that there is also an open PR (Updated docstring for binomial confidence intervals #55) for it. Is there any way of giving credit for the fix to @juliangehring ?

Thanks in advance for your input!

…eate_docstrings

BenjaminBorn · 2017-08-22T17:51:48Z

One idea for question (1) would be to put a "fake" general method in HypothesisTests.jl à la

"""
    confint(test::HypothesisTest, alpha = 0.05; tail = :both)

Compute a confidence interval C with coverage 1-`alpha`.

...

"""
confint(test::HypothesisTest, alpha = 0.05; tail = :both)

And similarly for pvalue. Would that be preferable to attaching it to a random method?

ararslan · 2017-08-22T20:30:45Z

A way to generally attach docstrings to functions is to either forward-declare the function with no methods, e.g.

"""
    confint(...)

...
"""
function confint end

or attach it to the function after methods have been defined for it, e.g.

"""
    confint(...)

...
"""
confint

nalimilan

Thanks for doing this, that's very useful! I've taken this opportunity to review the docstrings, so I've noted many small issues which were already present in the docs. Feel free to fix only those which are introduced by your PR, but if you feel like it it would be great to fix the others too. At any rate it's good to have a commit just to copy the existing docstrings, and one or more commits improving them, so that we can easily track the changes.

nalimilan · 2017-08-22T17:58:42Z

src/anderson_darling.jl

+`xs` comes from the same distribution against the alternative hypothesis that the samples
+comes from different distributions.
+
+`modified` paramater enables a modified test calculation for samples whose observations


"parameter". Do you have more details about what's that modified calculation?

nalimilan · 2017-08-22T18:08:31Z

src/t.jl

+    OneSampleTTest(xbar::Real, stdev::Real, n::Int, mu0::Real = 0)
+
+Perform a one sample t-test of the null hypothesis that `n` values with mean `xbar` and
+sample standard deviation `stdev`  come from a distribution with `mu0` against the


"with mean mu0". Also better use μ0 and stddev as in the code.

nalimilan · 2017-08-22T18:15:14Z

src/anderson_darling.jl

@@ -30,6 +30,15 @@ immutable OneSampleADTest <: ADTest
    A²::Float64 # Anderson-Darling test statistic
 end

+"""
+    OneSampleADTest{T<:Real}(x::AbstractVector{T}, d::UnivariateDistribution)


In general better do ::AbstractVector{<:Real} and remove T. Same elsewhere.

Only in the docstrings or should I also go through the function definitions?

You could do both if T isn't used in the body of functions, but to keep this PR focused better only touch docstrings.

nalimilan · 2017-08-22T18:19:00Z

src/anderson_darling.jl

+"""
+    KSampleADTest{T<:Real}(xs::AbstractVector{T}...; modified=true)
+
+Perform a k-sample Anderson–Darling test of the null hypothesis that the data in vectors


"in the k vectors" would be clearer. You can use double backticks around k for each of its uses.

nalimilan · 2017-08-22T18:19:37Z

src/anderson_darling.jl

+
+Perform a k-sample Anderson–Darling test of the null hypothesis that the data in vectors
+`xs` comes from the same distribution against the alternative hypothesis that the samples
+comes from different distributions.


"come". Same for previous occurrence (I guess).

nalimilan · 2017-08-22T21:45:23Z

src/t.jl

+    Most of the implemented confidence intervals are *strongly consistent*, that is, the
+    confidence interval with coverage 1-`alpha` does not contain the test statistic under
+    ``h_0`` if and only if the corresponding test rejects the null hypothesis
+    ``h_0: \\theta=\\theta_0``:


You can write alpha and theta directly using greek letters.

nalimilan · 2017-08-22T21:46:33Z

src/t.jl

+    EqualVarianceTTest(x::AbstractVector{T<:Real}, y::AbstractVector{T<:Real})
+
+Perform a two-sample t-test of the null hypothesis that `x` and `y` come from a
+distributions with the same mean and equal variances against the alternative hypothesis


"a distributions with the same mean and equal variances" -> "distributions with equal means and variances". Same below.

nalimilan · 2017-08-22T21:48:53Z

src/wilcoxon.jl

+When there are no tied ranks and ≤50 samples, or tied ranks and ≤15 samples,
+`SignedRankTest` performs an exact signed rank test. In all other cases, `SignedRankTest`
+performs an approximate signed rank test. Behavior may be further controlled by using
+`ExactSignedRankTest` or `ApproximateSignedRankTest` directly.


Use @ref.

nalimilan · 2017-08-22T21:49:22Z

src/mann_whitney.jl

+
+The Mann-Whitney U test is sometimes known as the Wilcoxon rank sum test.
+
+When there are no tied ranks and ≤50 samples, or tied ranks and ≤10 samples,


Use @ref.

nalimilan · 2017-08-22T21:49:44Z

src/wilcoxon.jl

+"""
+    ExactSignedRankTest(x::AbstractVector{T<:Real}[, y::AbstractVector{T<:Real}])
+
+Perform an exact signed rank U test.


Repeat the definition of the test. Same below.

nalimilan · 2017-08-22T21:56:33Z

I have attached the general description of pvalue() and confint() somewhat arbitrarily to two of the tests (Binomial.jl and t.jl). Is there a better place to put them?

The usual solution to that is to define function pvalue end somewhere and attach the generic docstring to that.

Sometimes a test has different methods, e.g. OneSampleTTest(). If possible, I have combined them in one docstring, but sometimes, for clarity reasons, I have included different docstrings for the methods. Is that ok, or should we combine them somehow into one docstring? The disadvantage of having them in different docstrings is that the help output in the REPL can get convoluted, especially as there is not much spacing between the output for the different methods.

I'd say that's OK as long as the repeated content is not too long. If the REPL output isn't clear, it should be improved, rather than working around it in packages (the HTML manual looks better in general).

pvalue and confint also have different methods with a number of separate docstrings. Is there a way to ensure that the general method, e.g. pvalue(x::HypothesisTests), is the first shown when typing ?pvalue? As some of the more specialised methods have long docstrings, it could get lost otherwise.

IIRC it depends on the order in which the methods are defined. Using the trick I gave above, you should be able to define it in the right place.

The admonition !!! note doesn't work in the Jupyter notebook (at least not for me using Safari). It works in the REPL and Juno. Maybe someone can try it. If it's a general problem, we can file an issue with IJulia (or at the appropriate place).

Agreed. Also check that it works in the HTML manual though.

There was a copy/paste mistake in the docstring for the binomial confidence intervals. After fixing it, I realised that there is also an open PR (#55) for it. Is there any way of giving credit for the fix to @juliangehring ?

Yes, the best way to give her credit is to merge here PR, which I just did. Thanks for pointing it out!

BenjaminBorn · 2017-08-23T11:49:28Z

Ok, I have (hopefully) addressed all points directly related to the PR. I have also updated the documentation more generally along the lines suggested by @nalimilan in a separate commit. However, I'm not able to answer the specific questions about the Power Divergence test so there, I only addressed the formatting issues. Could we deal with these specific questions in separate PR by someone more qualified?

ararslan · 2017-08-23T18:21:39Z

src/kruskal_wallis.jl

+The p-value is computed using a ``χ^2`` approximation to the distribution of the test
+statistic ``H_c=\\frac{H}{C}``:
+```math
+    \\begin{align}


I forget, does align number the lines and align* doesn't?

Good point, align numbers (although the Jupyter notebook doesn't print them). I'll change it to align* in the next iteration.

ararslan

Looks good to me. We can always improve docstrings later; that doesn't have to all happen at once here.

BenjaminBorn · 2017-08-23T18:30:52Z

I have attached the general description of pvalue() and confint() somewhat arbitrarily to two of the tests (Binomial.jl and t.jl). Is there a better place to put them?

The usual solution to that is to define function pvalue end somewhere and attach the generic docstring to that.

Thanks for the hint about zero-method functions. One related question. In Documenter.jl, it's possible to include a certain method via

```@docs
length(::T)
```

So if I now define a general docstring for pvalue(::HypothesisTest) using a zero-method function, there is no way of selecting this general definition via a method for the construction of the documentation, right? I tried

```@docs
pvalue(::HypothesisTest)
```

but that didn't work.

ararslan · 2017-08-23T18:33:28Z

```@docs
pvalue
```

🙂

BenjaminBorn · 2017-08-23T18:36:50Z

```@docs
pvalue
```

But that prints the docstrings for all defined methods of pvalue. What if I only want the general one? For example so that I can structure the documentation and put pvalue(::Binomial) somewhere else.

ararslan · 2017-08-23T18:53:06Z

Ah, I see. I think you should be able to do this:

"""
    pvalue(...)

Some general description
"""
pvalue(::HypothesisTest; kwargs...)

after methods have been defined for pvalue, rather than the 0-method forward declaration, then add pvalue(::HypothesisTest) or whatever to the @docs block.

BenjaminBorn · 2017-08-23T19:38:23Z

Thanks, but even if I put the definition at the end of HypothesisTests.jl after everything has been defined (and the docstring can be found, e.g. in the REPL), I still get

ERROR: LoadError: UndefVarError: HypothesisTest not defined
while loading /Users/bborn/.julia/v0.6/HypothesisTests/docs/make.jl, in expression starting on line 3

when I add pvalue(::HypothesisTest) to the @docs block.

ararslan · 2017-08-23T19:59:35Z

Hmmmmm, maybe

```@meta
CurrentModule = HypothesisTests
```

```@docs
pvalue(::HypothesisTest)
```

nalimilan · 2017-08-24T10:41:22Z

src/t.jl

+    OneSampleTTest(xbar::Real, stddev::Real, n::Int, μ0::Real = 0)
+
+Perform a one sample t-test of the null hypothesis that `n` values with mean `xbar` and
+sample standard deviation `stddev`  come from a distribution with `μ0` against the


Still need to fix "with mean μ0".

nalimilan · 2017-08-24T10:43:14Z

src/binomial.jl

+  - Agresti Coull interval `:agresti_coull`: Simplified version of the Wilson interval;
+    they are centered around the same value. The Agresti Coull interval has higher or
+    equal coverage.
+  - Arcsine transformation `:arcsine`.


OK, let's leave this for somebody else then. ;-)

nalimilan

Thanks! Looks almost ready, I've just noted a few remaining details.

nalimilan · 2017-08-24T10:50:48Z

src/power_divergence.jl

+
+If `x` is a matrix with at least two rows and columns, it is taken as a two-dimensional
+contingency table. Otherwise, `x` and `y` must be vectors of the same length. The contingency
+table is calculated using `counts` from [`Statsbase`](@ref). Then the power divergence test


"StatsBase". Also, I meant writing[`StatsBase.counts`](@ref) (I'm not sure a link to a module works).

The Documenter.jl documentation states

Note that depending on what the CurrentModule is set to, a docstring @ref may need to be prefixed by the module which defines it.

However, at least in my local build, links like [`StatsBase.counts`](@ref) don't seem to work. But maybe we can figure this out in the next PR that will actually set up Documenter.jl.

nalimilan · 2017-08-24T10:51:12Z

src/power_divergence.jl

+    /\\hat{n}_{ij})^λ -1\\right]
+```
+where ``n_{ij}`` is the cell count in the ``i`` th row and ``j`` th column and ``λ`` is a
+real number determing the nature of the test to be performed:


"determining"

nalimilan · 2017-08-24T10:53:01Z

src/power_divergence.jl

+"""
+    MultinomialLRT(x [,y] [,theta0])
+
+Convenience function for power divergence test with ``λ=0``.


Please at least copy the relevant docs from PowerDivergenceTest and fix the signature.

BenjaminBorn · 2017-08-24T11:59:51Z

Thanks @ararslan and @nalimilan for the thorough review. I have hopefully addressed all points now. Once this PR is merged, I will open a new PR to build the docs.

nalimilan

Just one more issue. Can you rebase and fix the conflicts so that I can merge?

nalimilan · 2017-08-24T12:44:24Z

src/fisher.jl

@@ -71,7 +71,8 @@ immutable FisherExactTest <: HypothesisTest
 end

 testname(::FisherExactTest) = "Fisher's exact test"
-population_param_of_interest(x::FisherExactTest) = ("Odds ratio", 1.0, x.ω) # parameter of interest: name, value under h0, point estimate
+population_param_of_interest(x::FisherExactTest) = ("Odds ratio", 1.0, x.ω)


Better not change this since that's unrelated. Or at least put the comment before the function rather than after. Same below.

Ok. I have reverted it.

BenjaminBorn · 2017-08-24T15:22:29Z

Done. Let me know if I should do anything else.

bjarthur · 2017-08-24T18:12:01Z

this is great! but... why is this package not being tested on julia 0.6?

ararslan · 2017-08-24T18:17:21Z

Will be. Once this is merged we should drop 0.5 support, test on 0.6, and have FemtoCleaner do a deprecation fixing pass.

ararslan · 2017-08-24T19:29:00Z

src/fisher.jl

+The one-sided p-values are based on Fisher's non-central hypergeometric distribution
+``f_ω(i)`` with odds ratio ``ω``:
+```math
+    \\begin{align}


I guess if you're moving everything to align* this should be as well? Aside from that, this all LGTM.

Thanks, fixed.

ararslan · 2017-08-24T19:43:53Z

src/fisher.jl

+|*Y2*| c  | d  |
+
+!!! note
+    The [`Base.show`](@ref) output contains the conditional maximum likelihood estimate of the odds ratio


When you build the docs, what does this end up linking to?

As noted above

The Documenter.jl documentation states
"Note that depending on what the CurrentModule is set to, a docstring @ref may need to be prefixed by the module which defines it."
However, at least in my local build, links like StatsBase.counts don't seem to work. But maybe we can figure this out in the next PR that will actually set up Documenter.jl.

So, at least for now the links to outside modules don't work. Those are:

[`StatsBase.counts`](@ref) and [`Base.show`](@ref) for which the error is that !! No doc found for reference

[`Rmath.pwilcox`](@ref) and [`Rmath.psignrank`](@ref) for which it says ERROR: UndefVarError(:Rmath), so this is maybe not the correct module call.

The Documenter.jl documentation unfortunately doesn't give an example how to link to other modules and I also couldn't find an example in other packages.

Besides these dead links, the docs are still built correctly and everything looks ok. If you have an idea on how to fix it, I can push the fix here. If it takes us longer to figure it out, we might want to merge here and then fix the links in the follow-up PR.

I'd just remove the dead ref links for now, e.g. in this case just say "The show output contains..."

Done. But you explain this to @nalimilan ;-)

Did Milan tell you to link to things that can't be properly linked...?

No no, of course not. I was joking. He told me to include the links but wasn't sure whether it would work. Let's leave them out and we can figure the linking out later.

Unfortunately it seems it's not possible at the moment: JuliaDocs/Documenter.jl#425. Let's go without links for now.

ararslan

This is good to go as far as I'm concerned. Thanks as always!

BenjaminBorn · 2017-08-24T20:46:45Z

Thanks Alex. Sorry for the back and forth.

ararslan · 2017-08-24T20:51:59Z

No problem at all! That's just how PR reviews go. You've done awesome work here, as usual.

ararslan · 2017-08-25T19:39:01Z

@nalimilan Good to merge? Any further cleanup that's necessary can always happen later.

nalimilan · 2017-08-26T18:29:08Z

Sorry for the delay. It's fine with me! Though conflicts have to be fixed first.

ararslan · 2017-08-26T18:30:46Z

Though conflicts have to be fixed first.

Weird, I'm not seeing any merge conflicts?

ararslan self-requested a review August 21, 2017 16:57

Move method descriptions to docstrings

9e0e6af

BenjaminBorn force-pushed the create_docstrings branch from da3b980 to 9e0e6af Compare August 21, 2017 17:19

Move method descriptions to docstrings

09aabcd

BenjaminBorn force-pushed the create_docstrings branch from 9e0e6af to 09aabcd Compare August 22, 2017 08:32

BenjaminBorn added 2 commits August 22, 2017 10:39

Move method descriptions to docstrings

eaec032

Merge remote-tracking branch 'BenjaminBorn/create_docstrings' into cr…

117e1fa

…eate_docstrings

BenjaminBorn mentioned this pull request Aug 22, 2017

Migrate documentation to Documenter #103

Closed

nalimilan reviewed Aug 22, 2017

View reviewed changes

Address review comments directly related to doc migration

5d9ba1f

General documentation updates

2caa23e

BenjaminBorn force-pushed the create_docstrings branch from f8f6b1a to 2caa23e Compare August 23, 2017 18:14

ararslan reviewed Aug 23, 2017

View reviewed changes

ararslan approved these changes Aug 23, 2017

View reviewed changes

Merge branch 'master' into create_docstrings

21d5741

nalimilan reviewed Aug 24, 2017

View reviewed changes

BenjaminBorn force-pushed the create_docstrings branch from 565bc79 to a02607a Compare August 24, 2017 15:20

ararslan reviewed Aug 24, 2017

View reviewed changes

BenjaminBorn force-pushed the create_docstrings branch from a02607a to c452f6b Compare August 24, 2017 19:32

ararslan reviewed Aug 24, 2017

View reviewed changes

Last fixes and rebase

84195ea

BenjaminBorn force-pushed the create_docstrings branch from c452f6b to 84195ea Compare August 24, 2017 20:26

ararslan approved these changes Aug 24, 2017

View reviewed changes

ararslan merged commit 6cc7d93 into JuliaStats:master Aug 26, 2017

BenjaminBorn deleted the create_docstrings branch August 26, 2017 18:44


		The Mann-Whitney U test is sometimes known as the Wilcoxon rank sum test.

		When there are no tied ranks and ≤50 samples, or tied ranks and ≤10 samples,

Move method descriptions to docstrings #106

Move method descriptions to docstrings #106

Conversation

BenjaminBorn commented Aug 21, 2017

BenjaminBorn commented Aug 22, 2017

ararslan commented Aug 22, 2017

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBorn Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Aug 22, 2017

BenjaminBorn commented Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan left a comment

Choose a reason for hiding this comment

BenjaminBorn commented Aug 23, 2017

ararslan commented Aug 23, 2017

BenjaminBorn commented Aug 23, 2017

ararslan commented Aug 23, 2017 • edited Loading

BenjaminBorn commented Aug 23, 2017

ararslan commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBorn commented Aug 24, 2017

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBorn commented Aug 24, 2017 • edited Loading

bjarthur commented Aug 24, 2017

ararslan commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan left a comment

Choose a reason for hiding this comment

BenjaminBorn commented Aug 24, 2017

ararslan commented Aug 24, 2017

ararslan commented Aug 25, 2017

nalimilan commented Aug 26, 2017

ararslan commented Aug 26, 2017

BenjaminBorn Aug 23, 2017 •

edited

Loading

BenjaminBorn commented Aug 23, 2017 •

edited

Loading

ararslan commented Aug 23, 2017 •

edited

Loading

BenjaminBorn commented Aug 24, 2017 •

edited

Loading