Adding offset functionality for stan_jm

[pamelanluna]

Implements an offset term for the longitudinal submodel. Resolves #405

[sambrilleman]

This looks really cool @pamelanluna! Thanks!

I especially like the introduction of scaling for the association parameter.

Is the contribution of the offset in the association structure just zero?

One small question, why is the scale_assoc incorporated here, rather than scaling somewhere like here. I'm probably missing the details, but I couldn't work out how the addition of log(scale_assoc) in the linear predictor for Y translated into scaling of the association parameter. Forgive me if I'm missing something simple.

If it's passing all the existing tests for the model estimates then I see no issue with merging these changes in.

[pamelanluna]

Thanks, @sambrilleman!

Yes, the offset is excluded/zero in the association structure. The idea here is that if you're adjusting for an offset term in the longitudinal submodel, you will want the adjusted model values in the event submodel. The inclusion of the offset term in the association structure is something that could be made optional, but I couldn't think of a use case where it'd be preferable.

I chose to include scale_assoc there because it touched less code, but you've found a bug in how I implemented the scaling. In my models I've been using the muvalue association structure, so scaling the linear predictor by adding log(scale_assoc) worked as expected. I will update the code to fix that issue.

[sambrilleman]

Great, thanks, sounds good!

(Makes sense about scale_assoc -- yeah I had assumed that the implementation was specific to a particular link function).

[bgoodri]

@sambrilleman Is this fine with you?

[sambrilleman]

@pamelanluna Did you end up pushing any commits that meant scale_assoc will work with any association structure (and not just muvalue)?

If not, then we might need to wait until that is done @bgoodri , because I don't think the offset will work with all association structures (atleast that is my interpretation from my earlier message from Feb).

[pamelanluna]

@sambrilleman No, these latest commits are just incorporating upstream changes into my fork for testing purposes. I am in the process of updating the scaling parameter code to work with other association structures, so these changes should be ready soon.

[sambrilleman]

@pamelanluna awesome, cheers for the update! 😃

[pamelanluna]

@sambrilleman Just wanted to make sure you saw that I had implemented these scaling parameter changes. Let me know if you have any questions or concerns.

[sambrilleman]

Hi @pamelanluna - sorry for the delay! This looks awesome and neatly done! 😀

A couple of questions:

1. Should we add a small test just to make sure that the scaling works as expected? For example take the example univariate joint model and fit it with scale_assoc = 0.5 and check that the estimated association parameter halves. I'm not certain that it would be particularly easy to implement that test though, because even with the same seed the introduction of the scaling parameter would mean that the parameter draws for the association parameter in the scaled model would not be exactly half of the original model. So might be more trouble than it is worth?

2. What (if anything) do we need to consider for prediction functions here? -- If a model is estimated with the scaled association parameter, then am I right in thinking that the predictions would also require the scaling parameter to be included?

[pamelanluna]

Thanks, @sambrilleman!

1. I added the simple test you suggested, and it produced the expected results. If something changes and the values are not quite equal, we can include a tolerance argument in the test for any slight variations in the results due to randomness.

2. Yes, the event model predictions account for the association scaling parameter in the .ll_survival function (called from posterior_survfit). The scaling parameter should not affect the longitudinal model predictions, as all of the scaling is handled in the event model. We might want to consider adding functionality to scale (or normalize for offset) the posterior trajectory plot, but I remember this being more complicated than expected when I looked at it previously.

[sambrilleman]

Awesome, sounds like you are all over it! 😀

Nothing else I can think of then.

@bgoodri @jgabry @pamelanluna I am happy for this to get merged if all tests are passing and everyone else is happy with it.

[jgabry]

Thanks @pamelanluna! @sambrilleman I'm definitely happy adding this feature, and if you're happy with the implementation then that's fine by me!

[pamelanluna]

@sambrilleman All tests are passing for me locally. Let me know if you need anything else from me to move forward with the merge. Thanks!

[pamelanluna]

@sambrilleman @jgabry @bgoodri Looks like everyone is happy with these changes. Can we get this merged?

[sambrilleman]

@pamelanluna All good from my end. Will be up to @bgoodri or @jgabry to action the merge, as they are maintainers of the repo. 😀

[bgoodri]

Thanks.

[jgabry]

@pamelanluna Sorry for the delay, I forgot we hadn't merged this. Thanks again for the PR!