-
Notifications
You must be signed in to change notification settings - Fork 59
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
rcopy handles formular symbols #524
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -38,6 +38,11 @@ | |||||
|
||||||
function rcopy(::Type{FormulaTerm}, l::Ptr{LangSxp}) | ||||||
expr = rcopy(Expr, l) | ||||||
# special case of a simple variable, like in aes(x, y) | ||||||
if Meta.isexpr(expr, :call) && length(expr.args) == 2 && expr.args[1] == :~ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was the concrete example which fails for me in current RCall. Having an expression like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in this example, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So to be honest, I don't actually know how RCall works internally. Is the following more or less accurate for this situation? The R surface syntax is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this PR, the behavior is now julia> using RCall
R> install.packages("^C
R> library("ggplot2")
julia> R"aes(x, y"^C
R> df <- data.frame(x=1,y=1)
julia> R"~x"
RObject{LangSxp}
~x
julia> rcopy(R"~x")
:(~x)
julia> rcopy(R"~y")
:(~y)
julia> rcopy(R"aes(df, x, y)")
OrderedCollections.OrderedDict{Symbol, Any} with 3 entries:
:x => :(~df)
:y => :(~x)
Symbol("") => :(~y)
julia> R"aes(df, x, y)"
RObject{VecSxp}
Aesthetic mapping:
* `x` -> `df`
* `y` -> `x`
* `` -> `y`
julia> rcopy(R"y~x")
FormulaTerm
Response:
y(unknown)
Predictors:
x(unknown)
julia> rcopy(R"~x+y")
:(~(x + y)) So this PR converts a one-sided formula into a symbol representation of that formula. @ararslan do you have an opinion on whether it would be better to strip the tilde? I'm a bit conflicted myself and could argue for either direction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any examples in mind of when stripping the tilde would not be desirable? If not then stripping it seems sensible to me, but otherwise would there still be a way to recover the original behavior in those cases after this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if there are cases where you would want to preserve a raw one-sided formula as an explicit one-sided formula. I can't think of any:
Going the other way: the tilde in one-sided formulae in R is generally analogous to I guess we can solve the dilemma by adding a note to the documentation that one-sided formulae in R are converted to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate the explanation, that all sounds good to me. I think the documentation aspect will be particularly important in case anybody ends up surprised by this behavior for whatever reason, however unlikely. Consider me on board with this change! |
||||||
return expr | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(so that we drop the tilde) |
||||||
end | ||||||
# complex formular | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
@eval StatsModels.@formula($expr) | ||||||
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.