Formula transfer updated to StatsModels v0.6.0 #317
Conversation
|
Any way we solve this (other than changing the tests) is going to require some fairly deep changes to StatsModels. Keeping the original formula expression around is a little tricky, since the formula macro doesn't actually explicitly create a call to If we want to make the formula more round-trippable (at least until |
|
bump. It would help to have this merged and a new release made. |
|
What's blocking here? I don't think it's worth waiting for changes to StatsModels. It's just not going to be possible to have full round-trip-ability of formulae between R and Julia I'm afraid because of the way that non-special function calls are handled |
|
@kleinschmidt I don't think there is anything blocking. I think it just fell through the cracks. I didn't want to go ahead and merge it without @simonbyrne or @randy3k taking a look at it first and I imagine they are both busy. I'm afraid I let it fall off of my ToDo list to remind them about it. |
|
@kleinschmidt I don't think anything is blocking here. I didn't want to merge without @simonbyrne or @randy3k taking a look and they are both busy, I assume. I know Simon started a new job in the last few months. I'm afraid I let prodding them slip off my ToDo list and I am trying to resurrect this. |
|
Sorry for saying the same thing twice. The first comment didn't appear initially so I thought I had somehow deleted it instead of hitting "Comment". |
| Formula(ex_orig, ex, lhs, rhs) | ||
| function rcopy(::Type{FormulaTerm}, l::Ptr{LangSxp}) | ||
| expr = rcopy(Expr, l) | ||
| @eval StatsModels.@formula($expr) |
There was a problem hiding this comment.
We shouldn't eval inside a function. Is there another interface we can use?
There was a problem hiding this comment.
I'm afraid not. At least not if we want to support calls to functions that are not part of the formula language (like log).
There was a problem hiding this comment.
the problem is that eval is called at top-level, so this probably won't actually work.
There was a problem hiding this comment.
I would be tempted to merge this PR and get a new version of RCall on the registry, even if it is likely that this will not work perfectly. Right now you can't install RCall if you want to have recent versions of GLM, MixedModels, etc. installed.
There was a problem hiding this comment.
I think it does work to call @eval in this case. At least the following simple test seems okay.
julia> rcopy(R"y ~ 1 + A + B + log(C)")
FormulaTerm
Response:
y(unknown)
Predictors:
1
A(unknown)
B(unknown)
(C)->log(C)There was a problem hiding this comment.
The question is if it works correctly from inside a function?
There was a problem hiding this comment.
How deeply inside a function do you need?
julia> using RCall
R> form <- y ~ 1 + A + B + log(C)
julia> function foo()
rcopy(R"form")
end
foo (generic function with 1 method)
julia> foo()
FormulaTerm
Response:
y(unknown)
Predictors:
1
A(unknown)
B(unknown)
(C)->log(C)
|
Any objections to my merging this? The CI failures are caused by the inability to install the R packages on a Mac for testing. |
Currently some of the tests are broken because the
StatsModels.@formulamacro does not preserve the original expression passed to it. The test could be changed to compare, e.g.to
which would look a little peculiar or, my preferred solution, create an
exorigfield in aFormulaTermlike that in aFunctionTerm. @kleinschmidt prefers not to do that. See discussion in #306