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

Issue 4: Non-parametric baseline hazard #313

Merged
merged 37 commits into from Sep 14, 2023
Merged

Issue 4: Non-parametric baseline hazard #313

merged 37 commits into from Sep 14, 2023

Conversation

seabbs
Copy link
Collaborator

@seabbs seabbs commented Aug 25, 2023

Description

This PR closes #4.

It adds a non-parametric baseline hazard model to allow the specification of @FelixGuenther original model or to get close to specifying @adrian-lison model. The current implementation does some non-optimal index hacks to get this working that I think should be resolved in a future PR (as indexing could in general be simplified). @kgostic this is likely useful for some of our applications but is obviously still a work in progress.

Work done:

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have updated the package development version by one increment in both NEWS.md and the DESCRIPTION.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@seabbs seabbs self-assigned this Aug 25, 2023
@github-actions
Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3881763 is merged into main:
These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2734ea8 is merged into main:
These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2734ea8 is merged into main:
These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@github-actions
Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2734ea8 is merged into main:
These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@github-actions
Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2734ea8 is merged into main:
These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@epinowcast epinowcast deleted a comment from codecov bot Sep 11, 2023
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #313 (ee533c3) into main (2734ea8) will increase coverage by 0.17%.
The diff coverage is 100.00%.

❗ Current head ee533c3 differs from pull request most recent head 8307946. Consider uploading reports for the commit 8307946 to get more accurate results

@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
+ Coverage   96.61%   96.78%   +0.17%     
==========================================
  Files          15       15              
  Lines        1771     1804      +33     
==========================================
+ Hits         1711     1746      +35     
+ Misses         60       58       -2     
Files Changed Coverage Δ
R/model-modules.R 99.50% <100.00%> (+0.58%) ⬆️

@github-actions
Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2734ea8 is merged into main:
These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

Copy link
Collaborator Author

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (self-review)

@seabbs
Copy link
Collaborator Author

seabbs commented Sep 13, 2023

The lintr CI check is failing when trying to install dependencies (specifically vdiffr) for unclear reasons that seem unrelated to this PR.

I've run lintr::lint_package() locally and fixed any issues that were flagged.

@github-actions
Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2734ea8 is merged into main:
These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

Copy link
Collaborator Author

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (self-review 2)

@seabbs
Copy link
Collaborator Author

seabbs commented Sep 14, 2023

I've done two rounds of self-review and am happy this is implemented approximately correctly (enough to be a rolling release candidate). Will work on #322 as a priority to help give extra confidence.

Given lack of review activity (which is fair enough) going to merge this but always happy to have a post merge review.

@seabbs seabbs merged commit e7c3ee9 into main Sep 14, 2023
9 of 10 checks passed
@seabbs seabbs deleted the seabbs/issue4 branch September 14, 2023 09:57
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.

Add non-parametric reference date model
2 participants