fabian-s
commented
8 years ago Thanks for figuring this out, Jonathan, makes a lot of sense to me.
But what happens to the smooth object, which we still need to keep track of? I guess we still need to handle this by passing xt() an environment (and an index) where the smooth object goes?
That seems unavoidable as long as xt() only takes an x argument and nothing else.
If we do this, then within xt() we can just search the environment for smooth, and if it is already present, call PredictMat instead of smoothCon - similar to what we are doing now. In this case, I don’t think we even have to write a makepredictcall() method, right?
Seems so.
It's great that all of this stuff seems to work now, but I'm also starting to think it might have been a better design choice to do the entire formula parsing / model matrix setup in our own code and basically only use coxph.fit instead of calling coxph and handing over all these intricate closures to make sure the terms get evaluated correctly. That would have probably meshed well with using makepredictcall as well. Too late now.... I really should have seen this coming, after having lots of similar issues when developing pffr by wrapping around gam instead of gam.fit :frowning:
jgellar
I’m trying to figure out how we could make
makepredictcall()work forpcox. Here is what I’ve learned about how it works:makepredictcall()is called withinmodel.frame.default. The first timemodel.frame.defaultis called (i.e., when the model is fit), thepredvarsattribute of the formula will beNULL. In this case, the columns of the model frame (i.e., the “variables”) are created by evaluating the “terms” calls.makepredictcall()is called on those variables. The result is a new call, which is stored as thepredvarsattribute of the formula.predict()is called on the model, model.frame.default is called again. This timepredvarsis not null. So instead of evaluating the “terms” call, it evaluates thepredvarscall.Now for
pcox…My initial assessment is that this will be a lot of work, and isn’t worth doing now because (a) we are pretty close to release as is, and (b) Therneau would need to implement some changes to the
ttsystem incoxphbefore this could even work. Assuming he follows through and does that though, it might be worth it in the end for us to do this because it will make all the methods magically work, perhaps more reliably/robustly to changes in thecoxphcode.Let’s first talk about the non time-varying terms, e.g. f(age), because these are the only ones we can even consider until (b) above is taken care of. What we want to happen is, the first time
model.frame.defaultis called, the “terms” call has to return acoxph.penaltyobject. We also need to setpredvars- this should be a call that ends up callingPredictMat()on the smooth object. However,PredictMatneeds adataargument when it is executed, andmodel.frame.defaultwon’t know to send the data in asdata. So we need to create a wrapper function that sets up the data, and callsPredictMatwith that argument.predvarswill be a call to this function. Luckily, we have a system to do this already - these are ourxtfunctions.Right now, all of the processing of these terms occurs within
pcox, beforecoxphis ever called. We convert the input data into acoxph.penaltyobject, and this is whatcoxphsees. So this won’t work withmakepredictcall- thesmoothobject never even makes it intocoxph, sopredvarswouldn’t be able to be set up right.What we would need to do is have all the processing occur within
coxph. I think this means that thecoxphcall that we create inpcoxwill have to have terms in it that are function calls essentially do what our currentxtfunctions do. e.g., if we call this functionxt(), then thecoxphcall is something likeCurrently, we create the
xtfunctions dynamically, so we don’t actually have a functionxt()in the package - we usecreate.xt.func(). This was done so the only argument of thextfunction we create isx, so that it mimics what we do forttfunctions (which are restricted to only have argumentsxandt). This would have to change for the new implementation. Instead of creating thextfunction dynamically, inpcoxwe set up a call to our newxt(), which does exactly what the dynamically-createdxtfunctions did before.So the first time
model.frame.defaultis called, thatxtterm will be executed and return acoxph.penaltyterm. That term will have to be of classxt(inheriting fromcoxph.penalty), so we can write amakepredictcall.xtmethod. But what happens to the smooth object, which we still need to keep track of? I guess we still need to handle this by passingxt()an environment (and an index) where thesmoothobject goes? If we do this, then withinxt()we can just search the environment forsmooth, and if it is already present, callPredictMatinstead ofsmoothCon- similar to what we are doing now. In this case, I don’t think we even have to write amakepredictcall()method, right?Let me know what you think (especially about this last part re: the smooth object).