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 917: Create a markdown of the synthetic recover results & post as a PR comment #928

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

kaitejohnson
Copy link
Contributor

@kaitejohnson kaitejohnson commented Jan 15, 2025

Description

This PR closes #917.

This PR creates a new synthetic_recovery.md which contains the results, in figure format, from running the inst/dev/recover-synthetic/rt.R and inst/dev/recover-synthetic/eval_rt.R. These scripts generate synthetic data and fit them using various specifications, and evaluate the results.

The modified .github/workflows/synthetic_validation.yml renders the markdown and posts a link to download the artifact as a comment. Another issue will be opened to see if we can avoid downloading and just link to a rendered html.

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

@kaitejohnson kaitejohnson changed the title Issue 917: Post artifact Issue 917: Create a markdown of the synthetic recover results & post as a PR comment Jan 15, 2025
@kaitejohnson
Copy link
Contributor Author

I'm not sure why this runs in my fork but not here... https://github.com/kaitejohnson/EpiNow2/actions/runs/12786541970/job/35647701957?pr=5

@sbfnk
Copy link
Contributor

sbfnk commented Jan 20, 2025

I'm not sure why this runs in my fork but not here... kaitejohnson/EpiNow2/actions/runs/12786541970/job/35647701957?pr=5

This seems to be a permission issue. The action reports that it can only read PRs
https://github.com/epiforecasts/EpiNow2/actions/runs/12786541962/job/35652034902?pr=928#step:1:27
where as e.g. the touchstone comment action can write
https://github.com/epiforecasts/EpiNow2/actions/runs/12863836932/job/35861169628#step:1:27

That said, it's not immediately obvious to me why these are different.

@kaitejohnson
Copy link
Contributor Author

Hmm yes on my fork the GH Action also has write permissions:
https://github.com/kaitejohnson/EpiNow2/actions/runs/12786541970/job/35647701957?pr=5

I'll look into the live codes or an alternative solution.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 20, 2025

Oh, I wonder if this is because you're trying to merge this from a fork - will test a PR within the repo.

@sbfnk sbfnk mentioned this pull request Jan 20, 2025
@sbfnk
Copy link
Contributor

sbfnk commented Jan 20, 2025

A-ha #930 (comment)

@kaitejohnson
Copy link
Contributor Author

Interesting, I didn't realise that permissions differ if you are opening a PR from a fork vs from within the repo! This now makes more sense, since my test was fully within my fork.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 20, 2025

@kaitejohnson given this works in the other PR, are you happy for this to be merged? I think the failing check on ubuntu is unrelated and it seems to happen across Actions.

@seabbs
Copy link
Contributor

seabbs commented Jan 20, 2025

note my comment about depending on the action used here.

@kaitejohnson
Copy link
Contributor Author

@kaitejohnson given this works in the other PR, are you happy for this to be merged? I think the failing check on ubuntu is unrelated and it seems to happen across Actions.

I think my main concern is the same as @seabbs, but I am not overly concerned about it as a solution since my understanding is we all sort of rely on GH actions from repos that may or may not be maintained.... (maybe this isn't true).

I am also pretty stuck on the LiveCodes thing (I think it being in java script is just making me really not understand what's going on.. kaitejohnson#8)

@sbfnk
Copy link
Contributor

sbfnk commented Jan 20, 2025

I think my main concern is the same as @seabbs, but I am not overly concerned about it as a solution since my understanding is we all sort of rely on GH actions from repos that may or may not be maintained.... (maybe this isn't true).

Is the worry that this will disappear? We could just copy it (with attribution) given that it's under an open license? Or is there some other concern?

@kaitejohnson
Copy link
Contributor Author

I think that makes sense! No I don't think there's any other concern from my perspective. Happy for it to be merged!

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.

better expose synthetic recovery
3 participants