Implement HypothesisCoding and ~un-export~ deprecate ContrastsCoding #158

kleinschmidt · 2019-10-02T15:26:33Z

ContrastsCoding is a footgun unless you really know what you're doing.
Hypothesis coding is a more natural way of specifying manual contrasts, since it
directly expresses differences between conditional means of levels in the matrix
(which is the pseudo-inverse of the contrasts matrix). This PR introduces a
HypothesisCoding <: AbstractContrasts which is exported, and un-exports
ContrastsCoding (and adds a note in the docs/help text that points to
hypotheses)

codecov-io · 2019-10-02T16:13:11Z

Codecov Report

Merging #158 into master will increase coverage by 0.35%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   86.11%   86.47%   +0.35%     
==========================================
  Files           9        9              
  Lines         497      510      +13     
==========================================
+ Hits          428      441      +13     
  Misses         69       69

Impacted Files	Coverage Δ
src/StatsModels.jl	`100% <ø> (ø)`	⬆️
src/contrasts.jl	`87.5% <93.75%> (+3.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51f1749...3fc0a8b. Read the comment docs.

kleinschmidt · 2019-10-02T16:17:47Z

I've also implemented sequential difference/sliding/repeated contrasts coding for the hell of it (see #120)

src/contrasts.jl

nalimilan · 2019-10-08T13:41:41Z

src/contrasts.jl

+Contrasts are derived the hypothesis matrix by taking the pseudoinverse:
+
+```jldoctest hyp
+julia> sdiff_contrasts = pinv(sdiff_hypothesis)


Suggested change

julia> sdiff_contrasts = pinv(sdiff_hypothesis)

julia> pinv(sdiff_hypothesis)

This isn't used anywhere but it is more to help with reading...

src/contrasts.jl

nalimilan · 2019-10-08T13:42:38Z

src/contrasts.jl

+
+"""
+mutable struct HypothesisCoding <: AbstractContrasts
+    hypotheses::Matrix


Shouldn't this be parameterized on the eltype?

Maybe, although it's going to get converted to whatever the eltype of the final model matrix is anyway. But maybe it would help the compiler?

Yeah, if the compiler can know the concrete type of such objects in typical situations (which I think is the case), it will generate much more efficient code. That should matter especially if you iterate over rows.

Though I've noticed this affects other contrasts, so maybe for a separate PR.

I think it only affect this and ContrastsCoding (the only other one that involves a manually specified matrix)

And anyway, I don't think the contrasts themselves ever get used in any hot code paths (that's all the ContrastsMatrix struct...)

nalimilan · 2019-10-08T13:44:35Z

src/contrasts.jl

+    base::Nothing
+    levels::Union{Vector,Nothing}
+
+    function HypothesisCoding(hypotheses, base, levels)


Why specify base if it's always nothing?

I think there's code that tries to access the field. I could overload getproperty for this type instead though, or refactor that code...

Since HypothesisCoding is new, how could existing code try to access that field? :-)

Anyway, in general I think fields are considered private unless specified otherwise. And unexporting ContrastsCoding is much more breaking. BTW, maybe we should keep exporting it for now until we want to make another breaking release? It's simpler to break many things at the same time than force people to update small things continuously.

Sounds good. Let's keep it exported but maybe add a warning to teh constructor or something?

I added a depwarn because the deprecations weren't quite doing it correctly (giving a spurious method overwritten warning)

The base field is used in the constructor of ContrastsMatrix. Could change that to use an accessor method but that seems like overkill at this point.

nalimilan · 2019-10-08T13:46:35Z

src/contrasts.jl

+Specify how to code a categorical variable in terms of a *hypothesis matrix*.
+For a variable with ``k`` levels, this should be a ``k-1 \times k`` matrix.
+Each row of the hypothesis corresponds to the hypothesis tested by the
+corresponding predictor, given as the weights given to the "cell mean" of each


I don't really understand that phrasing. The repetition of "given" doesn't help. :-)

how about "expressed as the weights of the "cell mean" of each of the k values of the predictor"?

Yeah... I guess I don't really understand what "cell means" refers to in this context, but maybe that's OK.

kleinschmidt · 2019-10-09T19:22:42Z

For non-breaking-ness, perhaps it would be better to deprecate ContrastsCoding with a message that it will be un-exported in the next minor release? Rather than removing it right now.

nalimilan · 2019-10-14T07:44:40Z

For non-breaking-ness, perhaps it would be better to deprecate ContrastsCoding with a message that it will be un-exported in the next minor release? Rather than removing it right now.

Yes, that sounds better, but I always forget whether that's possible or not. We could also just keep exporting it for now, and only remove it in the next minor release (when we want to break other, more fundamental things).

src/contrasts.jl

nalimilan · 2019-10-14T07:42:48Z

src/contrasts.jl

+Specify how to code a categorical variable in terms of a *hypothesis matrix*.
+For a variable with ``k`` levels, this should be a ``k-1 \times k`` matrix.
+Each row of the hypothesis corresponds to the hypothesis tested by the
+corresponding predictor, given as the weights given to the "cell mean" of each


Yeah... I guess I don't really understand what "cell means" refers to in this context, but maybe that's OK.

nalimilan

Looks good apart from the suggestion.

src/contrasts.jl

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

nalimilan reviewed Oct 8, 2019

View reviewed changes

nalimilan reviewed Oct 14, 2019

View reviewed changes

nalimilan approved these changes Feb 13, 2020

View reviewed changes

src/contrasts.jl Outdated Show resolved Hide resolved

kleinschmidt changed the title ~~Implement HypothesisCoding and un-export ContrastsCoding~~ Implement HypothesisCoding and ~~un-export~~ deprecate ContrastsCoding Feb 13, 2020

kleinschmidt changed the title ~~Implement HypothesisCoding and ~~un-export~~ deprecate ContrastsCoding~~ Implement HypothesisCoding and ~un-export~ deprecate ContrastsCoding Feb 13, 2020

kleinschmidt and others added 9 commits February 13, 2020 20:49

add HypothesisCoding, tests, don't export ContrastsCoding

6cadf3b

actually commit tests

9832d78

update docs

eae8dfd

add seqdiff coding, update docs and tests

0516d84

Apply suggestions from code review

5eb9148

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

outcome → outcomes

a833864

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

deprecate but export ContrastsCoding; clarify docstring

55b0d9e

fix typo

1de604e

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

bump patch version

3fc0a8b

kleinschmidt force-pushed the dfk/contrasts-update branch from a9743c2 to 3fc0a8b Compare February 13, 2020 19:50

kleinschmidt merged commit 0630f46 into master Feb 13, 2020

nalimilan deleted the dfk/contrasts-update branch February 13, 2020 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement HypothesisCoding and ~un-export~ deprecate ContrastsCoding #158

Implement HypothesisCoding and ~un-export~ deprecate ContrastsCoding #158

kleinschmidt commented Oct 2, 2019

codecov-io commented Oct 2, 2019 •

edited

Loading

kleinschmidt commented Oct 2, 2019

nalimilan Oct 8, 2019

kleinschmidt Oct 9, 2019

nalimilan Oct 8, 2019

kleinschmidt Oct 9, 2019

nalimilan Oct 14, 2019

kleinschmidt Feb 13, 2020

kleinschmidt Feb 13, 2020

nalimilan Oct 8, 2019

kleinschmidt Oct 9, 2019

nalimilan Oct 14, 2019

kleinschmidt Feb 13, 2020

kleinschmidt Feb 13, 2020

kleinschmidt Feb 13, 2020

nalimilan Oct 8, 2019

kleinschmidt Oct 9, 2019

nalimilan Oct 14, 2019

kleinschmidt commented Oct 9, 2019

nalimilan commented Oct 14, 2019

nalimilan Oct 14, 2019

nalimilan left a comment

	julia> sdiff_contrasts = pinv(sdiff_hypothesis)
	julia> pinv(sdiff_hypothesis)

Implement HypothesisCoding and ~un-export~ deprecate ContrastsCoding #158

Implement HypothesisCoding and ~un-export~ deprecate ContrastsCoding #158

Conversation

kleinschmidt commented Oct 2, 2019

codecov-io commented Oct 2, 2019 • edited Loading

Codecov Report

kleinschmidt commented Oct 2, 2019

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

kleinschmidt commented Oct 9, 2019

nalimilan commented Oct 14, 2019

Choose a reason for hiding this comment

nalimilan left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 2, 2019 •

edited

Loading