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 485: Update the phi parameterisation #486

Closed
wants to merge 12 commits into from

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Oct 24, 2023

Closes #485 and brings the vignette into line with code. As in the issue this has shown performance benefits in epinowcast though here we can't use inv_square as I think it is not yet in the latest version of rstan.

We could also change some variable names to make this all a bit clearer if we wish.

@seabbs seabbs requested a review from sbfnk October 24, 2023 16:37
@epiforecasts epiforecasts deleted a comment from github-actions bot Oct 24, 2023
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

Nice! Suggested some variable renaming as sqrt_phi doesn't really fit any more.

I hadn't realised touchstone doesn't work with branches in forks - worth pushing to a branch in the original remote and PRing again to see any noticable changes in efficiency?

inst/stan/functions/observation_model.stan Outdated Show resolved Hide resolved
inst/stan/functions/observation_model.stan Outdated Show resolved Hide resolved
inst/stan/functions/observation_model.stan Outdated Show resolved Hide resolved
inst/stan/functions/observation_model.stan Outdated Show resolved Hide resolved
inst/stan/functions/observation_model.stan Outdated Show resolved Hide resolved
inst/stan/functions/observation_model.stan Outdated Show resolved Hide resolved
inst/stan/functions/observation_model.stan Outdated Show resolved Hide resolved
@seabbs
Copy link
Contributor Author

seabbs commented Oct 24, 2023

I hadn't realised touchstone doesn't work with branches in forks - worth pushing to a branch in the original remote and PRing again to see any noticable changes in efficiency?

This is a PR from the original remote isn't it? I've just called the branch seabbs/update-phi-parameterisation?

@sbfnk
Copy link
Contributor

sbfnk commented Oct 24, 2023

I hadn't realised touchstone doesn't work with branches in forks - worth pushing to a branch in the original remote and PRing again to see any noticable changes in efficiency?

This is a PR from the original remote isn't it? I've just called the branch seabbs/update-phi-parameterisation?

Oh, of course. My bad. Puzzled what went here then https://github.com/epiforecasts/EpiNow2/actions/runs/6629882481/job/18010146022?pr=486#step:2:7905
Complete shot in the dark but perhaps there's an issue with slashes in branch names?

NEWS.md Outdated Show resolved Hide resolved
@seabbs
Copy link
Contributor Author

seabbs commented Oct 24, 2023

Complete shot in the dark but perhaps there's an issue with slashes in branch names?

Yes, potentially. We see transcient failures for epinowcast where that branch naming approach is pretty common - I haven't checked to see if its always branches with slashes that fail.

Co-authored-by: Sebastian Funk <[email protected]>
@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 151a8a0 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor

sbfnk commented Oct 25, 2023

Complete shot in the dark but perhaps there's an issue with slashes in branch names?

Yes, potentially. We see transcient failures for epinowcast where that branch naming approach is pretty common - I haven't checked to see if its always branches with slashes that fail.

lorenzwalthert/touchstone#125

@seabbs seabbs closed this Oct 25, 2023
@seabbs seabbs deleted the seabbs/update-phi-parameterisation branch October 25, 2023 10:13
@seabbs
Copy link
Contributor Author

seabbs commented Oct 25, 2023

Well that is good to know! I've closed and opened #487

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.

phi = 1 / sqrt(sqrt_phi) -> phi = 1 / sqrt_phi ^ 2
2 participants