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

Transition-specific costs and cCTSTMs #123

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

mclements
Copy link
Collaborator

Devin,

I have been working on a few changes to the hesim package.

For this pull request, I have implemented transition-specific costs, which currently only has an application for iCTSTMs.

Note that this folds in the unresolved inline extension from pull request #122.

As a signpost: I am preparing a presentation for the International Society of Clinical Biostatistics for later this month that implements cCTSTMs using ordinary differential equations. I have a gist and will prepare a pull request. I have a number of questions to ask you about that pull request:).

Sincerely, Mark.

@mclements
Copy link
Collaborator Author

I have added some changes to address the recent R check notes and warnings:

  • I incremented the version number
  • To remove a note about R6 not having any imports, I added @importFrom R6 R6Class in ctstm.R -- although this is not strictly necessary.
  • Most of the other notes had to do with documentation external links not having packages.

@mclements
Copy link
Collaborator Author

Devin: pkgdown is complaining about the documentation for CohortCtstm in ctstm.R. I can't see the error - would you be able to help here, please?

@mclements
Copy link
Collaborator Author

Some commentary on the implementation:

  • I was not able to find another package on CRAN that allows LinkingTo: for ordinary differential equations in C++. I investigated adapting the rehuel library for CRAN, but found that the package is not well maintained and some of the tests were failing. I have kept the BH dependency. For further discussion?
  • As discussed at The individual-based Markov CTSTM vignette uses a fixed time: is this sensible? #126, I have assumed that the point mass distribution is Markov. I am not happy with the implementation for the point masses: I needed dynamic_cast to determine (a) if a transmod was an mstate_list, and then (b) whether an element of mstate_list->survmod_ is a point mass (after adding an hesim::stats::distribution* get_dist(int trans) method to mstate_list). The code would be cleaner and clearer if we did not allow for point masses.
  • For the standard hip replacement vignette, I have assumed that the fifth transition is exponential. This is strictly Markov (clock-forward), whereas the individual-based vignette is a mix of clock-forward and clock-reset transitions.
  • I have some test code in preparation, but not completed.

@mclements
Copy link
Collaborator Author

I have now fixed the pkgdown issue.

@mclements
Copy link
Collaborator Author

@dincerti, I have now added some tests for this. This is now ready for review -- although admittedly this is a larger pull request than initially anticipated.

@mclements mclements requested a review from dincerti August 1, 2024 12:41
@mclements mclements changed the title Transition-specific costs Transition-specific costs and cCTSTMs Aug 1, 2024
@mclements mclements marked this pull request as draft August 8, 2024 15:18
@mclements
Copy link
Collaborator Author

mclements commented Aug 8, 2024

Devin:

  • I have now added an experimental cCTSTM implementation for semi-Markov models.
  • The implementation is similar to the Markov case, except that one needs to discretise for duration and keep track of both time in state and time in study.
  • However, the implementation is slow and needs +to be optimised.
  • TODO: I need to address some check warnings.

@mclements mclements marked this pull request as ready for review August 8, 2024 19:26
@mclements mclements marked this pull request as draft August 8, 2024 19:26
@mclements mclements marked this pull request as ready for review August 8, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant