Skip to content
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

Add additional links #170

Open
2 of 8 tasks
danielinteractive opened this issue Jul 6, 2023 · 17 comments
Open
2 of 8 tasks

Add additional links #170

danielinteractive opened this issue Jul 6, 2023 · 17 comments
Assignees
Labels
enhancement New feature or request Priority Is a priority issue
Projects

Comments

@danielinteractive
Copy link
Collaborator

danielinteractive commented Jul 6, 2023

To do:

  • lm-random-slope:
    • "shared random effect joint model", which means just the patient specific deviation from the common slope as link contribution (scaled by link coefficient)
  • GSF:
    • current longitudinal model value
    • growth rate parameter from the GSF
      • patient specific growth rate
      • individual deviation from population growth rate parameter
    • shrinkage rate parameter from the GSF
      • patient specific shrinkage rate
      • individual deviation from population shrinkage rate parameter
@danielinteractive danielinteractive added this to To do in Tasks via automation Jul 6, 2023
@gowerc
Copy link
Collaborator

gowerc commented Jul 25, 2023

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:

  • $s_i$ is the patient specific slope (= fixed slope + random slope)

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

@danielinteractive
Copy link
Collaborator Author

That is exactly correct

@gowerc gowerc added Priority Is a priority issue and removed complexity: Medium labels Aug 10, 2023
@gowerc gowerc added this to the Initial Internal Release milestone Sep 4, 2023
@gowerc gowerc added the Blocked Can't be advanced until another issue is solved label Oct 19, 2023
@gowerc
Copy link
Collaborator

gowerc commented Oct 23, 2023

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
Copy link
Collaborator

gowerc commented Feb 22, 2024

@danielinteractive Wondering about how to structure this.

For:

  • patient specific growth rate
  • patient specific shrinkage rate
  • Fixed slope

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 gowerc removed the Blocked Can't be advanced until another issue is solved label Feb 22, 2024
@gowerc
Copy link
Collaborator

gowerc commented Apr 8, 2024

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

@danielinteractive
Copy link
Collaborator Author

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
Copy link
Collaborator

gowerc commented Apr 10, 2024

@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
Copy link
Collaborator Author

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
Copy link
Collaborator

gowerc commented Apr 10, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

gowerc commented Apr 11, 2024

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
Copy link
Collaborator

gowerc commented Apr 12, 2024

@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
Copy link
Collaborator Author

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

@gowerc
Copy link
Collaborator

gowerc commented Apr 12, 2024

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
Copy link
Collaborator Author

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

@gowerc
Copy link
Collaborator

gowerc commented Apr 12, 2024

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
Copy link
Collaborator Author

You can decide, I guess we thought about it sufficiently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority Is a priority issue
Projects
Tasks
To do
Development

No branches or pull requests

2 participants