Bug-fix for respecting levels #91
Conversation
test/contrasts.jl
Outdated
| f = @formula(x ~ 1) | ||
| f = apply_schema(f, schema(data)) | ||
| y = modelcols(f.lhs, data) | ||
| f = apply_schema(f, schema(data, Dict(:x => DummyCoding(levels = levels(data.x))))) |
There was a problem hiding this comment.
Passing levels here is redundant and defeats the point of using a categorical array, right?
There was a problem hiding this comment.
It is redundant now, but works as a test since it errors with current master. That is just to verify that it is working as if/intended.
There was a problem hiding this comment.
OK but then below you need to check that y == modelcols(f.lhs, data) == somereference (or call @test y == somereference twice).
There was a problem hiding this comment.
Ah, and also better pass levels in a different order from levels and from the order of appearance, to ensure it really has an effect.
There was a problem hiding this comment.
And add an unused value to levels to check that it's dropped.
There was a problem hiding this comment.
That is what it is doing, it is first computing the value and storing it as y and the I test the value against the reference which is modelcols(f.lhs, data) with the formula now been updated with an explicit order by contrasts.
There was a problem hiding this comment.
The test for unused levels was already included in the modelmatrix.jl. I put a comment on that test that it also checks correctness against non-occurring levels.
There was a problem hiding this comment.
That is what it is doing, it is first computing the value and storing it as
yand the I test the value against the reference which ismodelcols(f.lhs, data)with the formula now been updated with an explicit order by contrasts.
Yes but this doesn't test the fact that passing levels to DummyCoding has any effect. Better check against an explicit expected array.
test/contrasts.jl
Outdated
| # Ordinal factors | ||
|
|
||
| data = DataFrame(x = levels!(categorical(['A', 'B', 'C', 'C'], | ||
| ordered = true), |
There was a problem hiding this comment.
Better not set ordered=true, as the behavior should be the same for unordered arrays. Then also change "ordinal" to "categorical" above.
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
=========================================
- Coverage 84.98% 84.49% -0.5%
=========================================
Files 8 8
Lines 453 458 +5
=========================================
+ Hits 385 387 +2
- Misses 68 71 +3
Continue to review full report at Codecov.
|
|
Let me know if there is anything else or if approved. |
|
Thanks! |
Allows contrasts to be aware of the order (especially important for ordinal features).