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 188: Add delay samples functionality #210

Merged
merged 17 commits into from
Aug 2, 2024
Merged

Issue 188: Add delay samples functionality #210

merged 17 commits into from
Aug 2, 2024

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented Jul 30, 2024

Description

This draft PR will close #188.

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 reviewed CI checks for this PR and addressed them as far as I am able.

@athowes
Copy link
Collaborator Author

athowes commented Jul 31, 2024

To do before merge:

@athowes athowes requested a review from seabbs July 31, 2024 15:00
@athowes athowes marked this pull request as ready for review July 31, 2024 15:00
@athowes
Copy link
Collaborator Author

athowes commented Jul 31, 2024

I think this is ready for review. I have created follow up issues.

Other thing is that for some reason it's failing a Ubuntu R-CMD-check but in a way that seems not related to the code changes. Any pointers?

@athowes athowes requested a review from kgostic July 31, 2024 15:01
@seabbs
Copy link
Contributor

seabbs commented Jul 31, 2024

The error is here:

Error ('test-unit-postprocess.R:5:3'): add_mean_sd.lognormal_samples works with posterior samples from the latent lognormal model ──
Error: Error: All list elements must have the same length.

You also have a few unrelated to this PR notes and warnings that we should flag and ideally fix

R/postprocess.R Outdated Show resolved Hide resolved
R/postprocess.R Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

This is looking good. Just naming to discuss I think

@athowes
Copy link
Collaborator Author

athowes commented Aug 1, 2024

Added issue about fixing possible check notes #217

@athowes
Copy link
Collaborator Author

athowes commented Aug 1, 2024

The error is here:

Error ('test-unit-postprocess.R:5:3'): add_mean_sd.lognormal_samples works with posterior samples from the latent lognormal model ──
Error: Error: All list elements must have the same length.

How did you see this? I can just see:

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-unit-postprocess.R:5:3'): add_mean_sd.lognormal_samples works with posterior samples from the latent lognormal model ──
Error in read_cmdstan_csv(files = self$output_files(include_failed = FALSE), variables = variables, sampler_diagnostics = sampler_diagnostics, format = format): Assertion on 'files' failed: File does not exist: '/tmp/RtmpVPgfDE/working_dir/Rtmpkb8cQq/model_ea52f49f447bb88d6fe4ace47415d149-202407312111-1-1098f0.csv'.

@athowes
Copy link
Collaborator Author

athowes commented Aug 1, 2024

Should we have functionality for getting a "properly" discretised PMF in this package or somewhere else?

@seabbs
Copy link
Contributor

seabbs commented Aug 1, 2024

How did you see this? I can just see:

I just looked through the log? What was the fix that made this go away?

@seabbs
Copy link
Contributor

seabbs commented Aug 1, 2024

Should we have functionality for getting a "properly" discretised PMF in this package or somewhere else?

My vote is somewhere else so that i.e epinowcast can use it and then also use it here but we could dev something here and then port out. Note that epinowcast already has a simulate based approach to this but really the approach taken in EpiAware (numeric integration) is the way to go I think

@athowes
Copy link
Collaborator Author

athowes commented Aug 1, 2024

How did you see this? I can just see:

I just looked through the log? What was the fix that made this go away?

I didn't do anything it just went away.

Copy link
Contributor

@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.

This is great. I think there are a few errors in the handling of newdata. I know we are doing the new data stuff separately but I think we need a few minimal checks here to be sure it works as is and avoid broken code going to main

R/postprocess.R Outdated Show resolved Hide resolved
R/postprocess.R Outdated Show resolved Hide resolved
@athowes
Copy link
Collaborator Author

athowes commented Aug 1, 2024

Good spot! Agree we should fix these newdata things.

@athowes
Copy link
Collaborator Author

athowes commented Aug 2, 2024

I've added tests for this (and indeed they did bring up another code issue! Which is now fixed).

@athowes athowes requested a review from seabbs August 2, 2024 10:37
Copy link
Contributor

@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.

Nice. Looks great.

@seabbs seabbs enabled auto-merge (squash) August 2, 2024 10:47
@seabbs seabbs merged commit a4ab006 into main Aug 2, 2024
6 of 7 checks passed
@seabbs seabbs deleted the delay-samples branch August 2, 2024 11:29
@athowes athowes mentioned this pull request Aug 9, 2024
3 tasks
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 functionality for generating individual or strata level delay distribution samples
2 participants