Genentech / jmpost

https://genentech.github.io/jmpost/
17 stars 4 forks source link

Add additional links #170

Open danielinteractive opened 1 year ago

danielinteractive commented 1 year ago

To do:

gowerc commented 1 year ago

Hey @danielinteractive , Would you mind clarifying what you mean by

"shared random effect joint model", which means just the patient specific deviation from the common slope as link contribution (scaled by link coefficient)

The current random-slope link contribution is:

$$ C_i(t \mid \phi, s_i) = \phi s_i $$

Where:

Am I right in thinking you mean an alternative as $s_i -$ fixed slope ?

danielinteractive commented 1 year ago

That is exactly correct

gowerc commented 10 months ago

Potentially blocked by #238 - Well its not blocked but it would be preferably to do 238 before doing this one to reduce the amount of backtracking required.

gowerc commented 5 months ago

@danielinteractive Wondering about how to structure this.

For:

I am wondering if its worth just doing a link_single_parameter() where the user can elect a singular TGI model parameter to be specified like

link = link_single_parameter("shrinkage")

I guess in theory the other 2 can be wrapped up in this frame work as well like:

link = link_single_parameter("shrinkage_deviance")

I guess this is a bit clunky though because you could argue well why isn't "identity" put under this framework as well... ?

gowerc commented 4 months ago

@danielinteractive - Apologies but would it be possible to get your thoughts on the above ?

danielinteractive commented 4 months ago

Hm that looks a bit clunky to me. I thought we have now methods for these links? I would rather like to see an API where each different link has another generic/method name.

gowerc commented 4 months ago

@danielinteractive - So if I was to implement this as distinct methods we would likely have:

link_growth()
link_growth_deviance()
link_shrinkage()
link_shrinkage_deviance()
link_slope_deviance()

I guess my reservation is that these are quite model specific e.g. it will be a generic for a single method implementation and it will be quite clunky to know which methods work with which models outside of digging into the documentation. I mean, having said that I'm not sure I have a good alternative...

Having written that, would I be right in saying that in the case of the random slope model link_growth() would just be the same as the random slope paramter e.g. link_dsld()?

danielinteractive commented 4 months ago

Yes, I would recommend going this route. You can easily query using standard R methods stuff what works where.

Yes to your specific question.

gowerc commented 4 months ago

I should add though you can't use the standard method querying stuff because these user facing functions are actually just general functions that wrap the underlying generics (this was done to provide a convenience layer for users to ease the UI). E.g. the users don't interact with the true generics which are linkDSLD() / linkTTG()

danielinteractive commented 4 months ago

Hm, I see... and we cannot / don't want to expose the true generics?... Just wondering if that would be long term easier to maintain as well

gowerc commented 4 months ago

At the time there was a very good reason.... I'm struggling to remember it now though, it was technical in nature. I'm going to double check the notes to see if I can remember why as I agree it would be cleaner if we just had straight generic methods being specified by the user...

gowerc commented 4 months ago

@danielinteractive , From what I can tell I did this as a UI/UX trade off. Originally I wanted to avoid the user having to manually execute the generics themselves as I was worried it was a bit clunky from a syntax point of view, for example I was trying to avoid:

longmodel <- LongitudinalRandomSlope()
JointModel(
    survival = SurvivalWeibull(),
    longitudinal = longmodel,
    link = Link(
        linkDSLD(longmodel, prior = prior_normal()),
        linkTTG(longmodel, prior = prior_normal()),
        linkIdentity(longmodel, prior = prior_normal())
    )
)

Ideally what I would want this to be is something more like...

JointModel(
    survival = SurvivalWeibull(),
    longitudinal = LongitudinalRandomSlope(),
    link = Link(
        linkDSLD,
        linkTTG,
        linkIdentity
    )
)

The problem with this though is prior specification. If you are passing unevaluated generic functions as inputs then there is no way for the users to specify the priors. So instead I opted for closure functions for the generics that enclose the prior e.g.

JointModel(
    survival = SurvivalWeibull(),
    longitudinal = LongitudinalRandomSlope(),
    link = Link(
        link_dsld(prior = prior_normal()),
        link_ttg(prior = prior_gamma()),
        link_identity(prior = prior_normal())
    )
)

Where roughly:

link_dsld <- function(prior) {
    function(model) {
        linkDSLD(model, prior)
    }
}

Basically if we want the user to pass the generic functions themselves as inputs then we need the user to evaluate the generic against the model themselves so they can pass a prior as well.

Going over all this again I'm not actually sure what I prefer... I don't like the fact we have 2 layers of functions as this is complicated to understand from a extending point of view and makes method discovery difficult. But I also don't like getting the user to have to invoke the generic themselves as this is clunky syntax and potentially an error source if users copy code forward and evaluate the generic against the wrong model objects.

danielinteractive commented 4 months ago

Hm I think it would be ok to pass longmodel as a user. It conceptually is well understandable.

gowerc commented 4 months ago

Ok after playing around with this quite a bit I think I found a solution that gets the best of both worlds. Some toy example code to demonstrate the point:

.longmodel <- setClass("longmodel", slots = c(code = "character"))
.prior <- setClass("prior", slots = c(mu = "numeric"))
.linkcomponent <- setClass("linkcomponent", slots = c(prior = "prior", code = "character"))

linkDsld <- function(prior, model = NULL, ...) {
    UseMethod("linkDsld", model)
}

linkDsld.default <- function(prior, model, ...) {
    function(model) {
        linkDsld(prior, model = model)
    }
}

linkDsld.longmodel <- function(prior, model, ...) {
    .linkcomponent(
        code = "my code for this object",
        prior = prior
    )
}

jointmodel <- function(longitudinal, link) {
    link(longitudinal)
}

jointmodel(
    .longmodel(code = "longitudinal model code"),
    linkDsld(prior = .prior(mu = 2))
)

Essentially if the user doesn't provide a model argument to the link method then it falls to the default method which returns a closure that has the prior embedded in it which just calls itself again. This way we can still get the user to pass around the generic functions and still see the available methods by the normal means

danielinteractive commented 4 months ago

I would still question whether this warrants the additional complexity...

gowerc commented 4 months ago

Personally I really dislike the idea of the user having to potentially specify the longitudinal model is so many places:

longmodel <- LongitudinalRandomSlope()
JointModel(
    survival = SurvivalWeibull(),
    longitudinal = longmodel,
    link = Link(
        linkDSLD(longmodel, prior = prior_normal()),
        linkTTG(longmodel, prior = prior_normal()),
        linkIdentity(longmodel, prior = prior_normal())
    )
)

My gut feeling just says this is clunky and likely to be error prone.

danielinteractive commented 4 months ago

You can decide, I guess we thought about it sufficiently