-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial Release #3
Conversation
<!-- | ||
## Examples | ||
|
||
```julia | ||
using RegressionFormulae, StatsModels, GLM, DataFrames | ||
|
||
``` | ||
--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example is commented out for now because examples are nice, but ain't nobody got time to write nice ones.
"a & b & c", "a & b & d", "a & c & d", "b & c & d"] | ||
|
||
m = fit(DummyMod, @formula(y ~ (1 + a + b + c + d)^3), dat) | ||
@test_broken coefnames(m) == ["(Intercept)", "a", "b", "c", "d", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should avoid computing interactions with the intercept, but I'm not sure if this is a "garbage in, garbage out" scenario or whether we should be more helpful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently that check is handled at parse time (with the And1
rule + uniqueness). For run-time, I think this is fixed in https://github.com/JuliaStats/StatsModels.jl/pull/183/files?authenticity_token=tpSCakRCz6UcTo6%2FFiR9px0AGprzcJ%2BMuaQNmJ%2FJkVQ93j9Rtc8kuPjoDQMS1l%2BMKT1YlkQnSh1xmLi5uo8f%2BQ%3D%3D&file-filters%5B%5D=.jl#diff-2fdd29708d32007e0664535695f51d68832983538a5fc2526a34ae1cf26e5361R384-R385
@testset "embedded interactions" begin | ||
m = fit(DummyMod, @formula(y ~ (a + b + c * d)^3), dat) | ||
cn = coefnames(m) | ||
@test_broken !("a & c & c & d" in cn) | ||
@test_broken cn == unique(cn) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken tests to highlight broken functionality 😉
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one design decision to make here is whether to actually enforce the expectation that the grouping term in /
will be full dummy. If we're generating a + a & b
from a/b
then a
SHOULD be promoted to full dummy by the normal mechanisms if b
isn't specified somewhere else. When b
IS specified somewhere else, manually specifying full dummy will lead to malformed models...
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91" | ||
StatsModels = "3eaba693-59b7-5ba5-a881-562e759f1c8d" | ||
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" | ||
|
||
[targets] | ||
test = ["Test"] | ||
test = ["StatsBase", "StatsModels", "Test"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're not supporting <1.6 twe could just use the test/Project.toml
approach instsead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought about doing that, but given the small set of dependencies for the tests, I just stayed this way. If we wind up with more extensive dependencies, then I would probably change over
import StatsModels: apply_schema | ||
using StatsModels: apply_schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just need to make sure that it's qualified when we're overloading those methods...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are 😉
Co-authored-by: Dave Kleinschmidt <[email protected]>
Yeah, I think there are some really weird edge cases with some of these things that we're only really going to figure out when we see them used in practice. I think our current approach -- |
Supercedes #2 as a local pull.
@kleinschmidt todo
main
and only allow squash merging.