add proptable #19

bkamins · 2017-12-19T14:42:23Z

following #8.

nalimilan · 2017-12-19T15:22:37Z

Thanks. Looks good, I just wonder about the name dims. Are there other examples of functions which take a keyword argument indicating a region like that? It would be nice to follow a general convention. Also, the plural is weird when you pass a single dimension (which is the most common case), so maybe just dim? I wish we could make it a positional argument...

bkamins · 2017-12-19T16:04:39Z

An attempt for positional argument. Loos better?

nalimilan · 2017-12-19T17:39:25Z

Clever! It's tempting, but it's also very non-standard so I'm really hesitant. Can you have a look at what similar functions in Base or in packages do in similar cases?

bkamins · 2017-12-19T19:30:29Z

sum, maximum, minimum, all, any, prod: call it dims in documentation;
mean, reducedim, mapreducedim call it region in documentation.

In all cases those are positional arguments. I have not found the use of Vararg the way I proposed.

Finally the option is to define proptable to already work on frequency table (or any other AbstractArray), so one would write something like:

proptable(freqtable(x, y), 1)

then the definition is even simpler:

proptable(tbl::AbstractArray) = tbl / sum(tbl)
proptable(tbl::AbstractArray, dims) = tbl ./ sum(tbl, dims)

This is the way R works. Actually, when I wanted proptable I almost never also needed freqtable in R - that is why I proposed to squash it into one function, but we can split it.

coveralls · 2017-12-19T19:54:55Z

Coverage increased (+0.5%) to 96.053% when pulling 4eda4f9 on bkamins:proptable into 0d70f4a on nalimilan:master.

coveralls · 2017-12-20T20:26:15Z

Coverage increased (+0.5%) to 96.053% when pulling 6268227 on bkamins:proptable into 0d70f4a on nalimilan:master.

nalimilan · 2017-12-20T20:50:54Z

Yeah, implementation-wise it's simpler to require proptable(freqtable(...), dims), but I have the same experience as you regarding the fact that you very often (though not always) call both at the same time. I think it would still make sense to have that function, since it happens that you have a table of counts an want the proportions. Maybe that function could just be called prop, as I'm not sure what "table" adds here.

It's still tempting to add a direct way to compute a table of proportions, either via a dedicated function or via a keyword argument. Or maybe we should wait until we add a more general convenience function, like Stata's tabulate or R's gmodels::CrossTable.

bkamins · 2017-12-20T21:34:18Z

The way to go probably depends on the way you want FreqTables to be developed (actually if you would outline what else you would see in this package I could contribute - I work with such type of analysis a lot).

For me things like tabulate or CrossTable or SAS PROC FREQ seem too heavy for this package.
I guess you do not want to put statistical tests etc. into this package as there are other packages for this - right?
Also I prefer UNIX-like approach of design based on simple functions doing one thing.

Given the above - if we named the function prop (or maybe props would be better?) I would like it as an adequate solution.
After thinking about it this .table part in R was bugging me with prop.table the most 😄.
If we went this way the question is how do you feel that the type of tbl should be restricted - any AbstractArray as above or something more specific, like AbstractArray{<:Integer}?

nalimilan · 2017-12-21T07:34:19Z

The way to go probably depends on the way you want FreqTables to be developed (actually if you would outline what else you would see in this package I could contribute - I work with such type of analysis a lot).

For me things like tabulate or CrossTable or SAS PROC FREQ seem too heavy for this package.
I guess you do not want to put statistical tests etc. into this package as there are other packages for this - right?
Also I prefer UNIX-like approach of design based on simple functions doing one thing.

The scope of the package isn't completely defined. Originally I hoped that the functions would be included in StatsBase once we agreed on a standard for named arrays/axis arrays, but that hasn't happened (yet?). Anyway I wouldn't say that these would be too heavy: by what definition are they "heavy" for a package dedicated to frequency tables? :-) As for tests, I'd rather put them in HypothesisTests, at least if they can support NamedArray inputs. We really need an equivalent of the simple chisq.test R function somewhere.

Given the above - if we named the function prop (or maybe props would be better?) I would like it as an adequate solution.
After thinking about it this .table part in R was bugging me with prop.table the most 😄.
If we went this way the question is how do you feel that the type of tbl should be restricted - any AbstractArray as above or something more specific, like AbstractArray{<:Integer}?

Let's go with prop then, anyway that's a good first step even if we decide to add the other approach too. I'm not a fan of the plural form, but if you have arguments for it... I guess it should accept any AbstractArray{<:Number}?

bkamins · 2017-12-21T12:14:17Z

I have commited prop. I was afraid of such a short name for a function working on a type from Base, but I have checked and there should not be much confusion (in the past prop was used to indicate property sometimes).

As for other things - I put chisq.test on my to do 😄.

nalimilan · 2017-12-21T13:49:31Z

README.md

@@ -44,6 +53,15 @@ b           │ 3  2
 c           │ 2  3
 d           │ 2  3

+julia> prop(tbl2, 1)


This behavior (inherited from sum) is the opposite of R's: the passed dimensions are those to collapse, while in R they are ones to retain. I wonder whether it's appropriate for computing proportions: it's probably more natural to think "I want to compute proportion by rows" than "I want to divide each row by the column sums", isn't it? After all, we say "row profiles/percents" and "column profiles/percents".

I was thinking about it, initially I wanted to be consistent with sum.
But let us call the argument margin then and make it work like in R.

nalimilan · 2017-12-21T13:49:50Z

src/freqtable.jl

@@ -131,3 +131,6 @@ function freqtable(d::AbstractDataFrame, x::Symbol...; args...)
    setdimnames!(a, x)
    a
 end
+
+prop(tbl::AbstractArray{<:Number}) = tbl / sum(tbl)


Could you add a docstring?

I wanted to add a docstring for freqtable and prop in a separate PR when we have settled the implementation.

nalimilan · 2017-12-21T19:02:21Z

src/freqtable.jl

 prop(tbl::AbstractArray{<:Number}) = tbl / sum(tbl)
-prop(tbl::AbstractArray{<:Number}, dims) = tbl ./ sum(tbl, dims)
+
+function prop(tbl::AbstractArray{<:Number,N}, margin) where N


Why not use margin::Integer...? Also, what's minimum?

Integer... - good idea, changing
minimum - removed, I wanted to check for negative entries but decided that it is not needed

nalimilan · 2017-12-21T19:03:05Z

src/freqtable.jl

+        throw(ArgumentError("invalid margin argument; expected integers in a valid dimension range"))
+    end
+    minimum
+    tbl ./ sum(tbl, tuple(setdiff(1:N, margin)...))


Is that inferrable? It would be nice to add @inferred to tests. If not, I wonder whether we could figure out a solution based on dispatch.

it is; added check to tests

nalimilan · 2017-12-21T19:04:00Z

src/freqtable.jl

+"""
+    prop(tbl, [margin])
+
+Create table of proportions from a frequency table `tbl` with margins generated for


Not necessarily a frequency table: any numeric table will do.

nalimilan · 2017-12-21T19:04:59Z

src/freqtable.jl

+**Examples**
+
+```julia
+prop([1 2; 3 4])


Better use jldoctest and put the full REPL output.

nalimilan · 2017-12-21T19:05:14Z

src/freqtable.jl

+prop([1 2; 3 4], (1,2))
+tbl = prop(rand(5, 5, 5, 5), (1, 2))
+sumtbl = sum(tbl, (3,4))
+all(x -> x ≈ 1.0, sumtbl)


No need to show that, just print the output of sum.

OK given the change above

nalimilan · 2017-12-21T19:07:52Z

src/freqtable.jl

+Calculating `sum` over the result of `prop` over dimensions that are complement of `margin`
+produces `AbstractArray` containing only approximately `1.0`, see last example below.
+
+**Arguments**


The docstring recommendations in the manual say that we should avoid using a separate section for arguments then possible. Here, it would be clearer to mention the expected types of arguments in the signature, and explain what they do directly in the description.

It would also be be nice to be less technical and explain in very simple terms how to compute row and column proportions. No need to mention "approximately" nor duplicated entries.

although margin allows to many types to specify it in the signature. I left it in the description.

sorry - given your comment below I can put it into the signature :)

coveralls · 2017-12-21T19:47:53Z

Coverage increased (+0.3%) to 95.89% when pulling a9070ae on bkamins:proptable into 0d70f4a on nalimilan:master.

coveralls · 2017-12-21T19:47:54Z

Coverage increased (+0.3%) to 95.89% when pulling a9070ae on bkamins:proptable into 0d70f4a on nalimilan:master.

nalimilan · 2017-12-21T20:16:45Z

test/freqtable.jl

@@ -26,7 +59,10 @@ tab =freqtable(x, y,
              3.0 2.0
              1.5 1.0]
 @test names(tab) == [["a", "b", "c", "d"], ["C", "D"]]
-
+@test prop(tab) == [4 6; 2 3; 6 4; 3 2] / 30.0


Wouldn't hurt to have @inferred here too.

nalimilan · 2017-12-21T20:16:49Z

src/freqtable.jl

+ 1.0  1.0
+ 1.0  1.0
+
+julia> tbl = prop(rand(2, 2, 2, 2), 1, 2); sum(tbl, (3,4))


Could you show this with a simple 3D array, e.g. reshape(1:12, (2, 2, 3)), and print the result of prop first? I think that's very helpful to understand what this does.

nalimilan · 2017-12-21T20:42:51Z

test/freqtable.jl

+             0.075  0.05  0.05 0.075;
+              0.05 0.075 0.075  0.05;
+              0.05 0.075 0.075  0.05]
+pt = @inferred prop(tab, 2)


Looks like this test fails. Maybe inference doesn't work after all?

Good catch. I tested it on Vector and it works. It fails NamedArray - I will investigate what can be done.

Ah, weird. Looks like the problem might be in sum(a::NamedArray, dim):

julia> @code_warntype sum(NamedArray([1 2; 3 4]), 1) Variables: #self# <optimized out> a::NamedArrays.NamedArray{Int64,2,Array{Int64,2},Tuple{DataStructures.OrderedDict{String,Int64},DataStructures.OrderedDict{String,Int64}}} d::Int64 Body: begin return $(Expr(:invoke, MethodInstance for sum(::NamedArrays.NamedArray{Int64,2,Array{Int64,2},Tuple{DataStructures.OrderedDict{String,Int64},DataStructures.OrderedDict{String,Int64}}}, ::Tuple{Int64}), :(NamedArrays.sum), :(a), :((Core.tuple)(d)::Tuple{Int64}))) end::Any

It is - I have patched it here, but see the comment below.

bkamins · 2017-12-21T22:07:16Z

In order to ensure type stability I had to use internals of NamedArrays that are not exported.
In general constructors of NamedArray are not type stable:

julia> @code_warntype NamedArray([1,2,3])
Variables:
  #self#::Type{NamedArrays.NamedArray}
  array::Array{Int64,1}

Body:
  begin
      SSAValue(0) = (Core.tuple)((Base.arraysize)(array::Array{Int64,1}, 1)::Int64)::Tuple{Int64}
      return $(Expr(:invoke, MethodInstance for NamedArrays.NamedArray(::Array{Int64,1}, ::Array{Array{String,1},1}, ::Array{Symbol,1}), :(#self#), :(array), :($(Expr(:invoke, MethodInstance for collect(::Base.Generator{Tuple{Int64},NamedArrays.#defaultnames}), :(Base.collect), :($(Expr(:new, Base.Generator{Tuple{Int64},NamedArrays.#defaultnames}, :($(QuoteNode(NamedArrays.defaultnames))), SSAValue(0))))))), :($(Expr(:invoke, MethodInstance for collect(::Base.Generator{UnitRange{Int64},NamedArrays.#defaultdimname}), :(Base.collect), :($(Expr(:new, Base.Generator{UnitRange{Int64},NamedArrays.#defaultdimname}, :($(QuoteNode(NamedArrays.defaultdimname))), :($(Expr(:new, UnitRange{Int64}, 1, :((Base.select_value)((Base.sle_int)(1, 1)::Bool, 1, (Base.sub_int)(1, 1)::Int64)::Int64))))))))))))
  end::Any

coveralls · 2017-12-22T00:39:02Z

Coverage increased (+0.3%) to 95.89% when pulling a660551 on bkamins:proptable into 0d70f4a on nalimilan:master.

coveralls · 2017-12-22T00:39:02Z

Coverage increased (+0.3%) to 95.89% when pulling a660551 on bkamins:proptable into 0d70f4a on nalimilan:master.

nalimilan · 2017-12-22T08:53:13Z

OK. Do you think there would be a way to make these constructors type-stable in NamedArrays itself? That would be useful in general.

bkamins · 2017-12-22T09:48:54Z

If it does not get fixed earlier I put it to my TODO, but I have some other things I would like to patch before :).

Anyway - even if NamedArrays are fixed the code I propose should be optimal (unless internal representation of NamedArrays is changed, because then it will be broken).

bkamins · 2017-12-22T09:50:10Z

BTW: do you know why in CI NamedArrays fail to precompile on 0.7?

nalimilan · 2017-12-22T10:17:23Z

Let's go with that, a two-line function isn't too bad as a temporary solution. But we really need to fix these constructors, as performance suffers a lot.

BTW: do you know why in CI NamedArrays fail to precompile on 0.7?

ERROR: LoadError: LoadError: UndefVarError: writedlm not defined

0.7 is going to require lots of small changes.

nalimilan · 2017-12-22T10:22:55Z

I've just tagged a release: JuliaLang/METADATA.jl#12595

add proptable

961525e

bkamins force-pushed the proptable branch from 4d64637 to 9396cb4 Compare December 19, 2017 16:04

bkamins force-pushed the proptable branch 2 times, most recently from 4eda4f9 to 7c63df4 Compare December 19, 2017 19:14

refactor arguments

6268227

bkamins force-pushed the proptable branch from 7c63df4 to 6268227 Compare December 20, 2017 19:07

introduce prop

f65d113

nalimilan reviewed Dec 21, 2017

View reviewed changes

add docstring and change dims to margin

a9070ae

nalimilan reviewed Dec 21, 2017

View reviewed changes

change prop signature and refactor docstrings

4e715b6

nalimilan reviewed Dec 21, 2017

View reviewed changes

improved tests and docs

495ce57

nalimilan reviewed Dec 21, 2017

View reviewed changes

patch NamedArrays

a660551

bkamins mentioned this pull request Dec 21, 2017

Type stability of NamedArrays davidavdav/NamedArrays.jl#58

Open

nalimilan merged commit 0392f03 into nalimilan:master Dec 22, 2017

nalimilan mentioned this pull request Mar 21, 2019

prop not working with categorical arrays? #37

Closed

nalimilan pushed a commit that referenced this pull request Apr 9, 2019

add proptable #19 (#38)

298a655

bkamins mentioned this pull request Nov 8, 2019

marginal values #8

Open

add proptable #19

add proptable #19

Conversation

bkamins commented Dec 19, 2017

nalimilan commented Dec 19, 2017

bkamins commented Dec 19, 2017

nalimilan commented Dec 19, 2017

bkamins commented Dec 19, 2017

coveralls commented Dec 19, 2017

coveralls commented Dec 20, 2017 • edited Loading

nalimilan commented Dec 20, 2017

bkamins commented Dec 20, 2017

nalimilan commented Dec 21, 2017

bkamins commented Dec 21, 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

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

Choose a reason for hiding this comment

coveralls commented Dec 21, 2017

coveralls commented Dec 21, 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

bkamins commented Dec 21, 2017

coveralls commented Dec 22, 2017

coveralls commented Dec 22, 2017

nalimilan commented Dec 22, 2017

bkamins commented Dec 22, 2017

bkamins commented Dec 22, 2017

nalimilan commented Dec 22, 2017

nalimilan commented Dec 22, 2017

coveralls commented Dec 20, 2017 •

edited

Loading