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 with the results of the synthetic recovery runs #925

Closed
wants to merge 37 commits into from

Conversation

kaitejohnson
Copy link
Contributor

@kaitejohnson kaitejohnson commented Jan 13, 2025

Description

This PR closes #917 (part of it)

It creates a synthetic_recovery.md which uses the figures generated in inst/dev/recover_synthetic/rt.R and inst/dev/recover_synthetic/eval_rt.R to create visualisations of the fits to simulated data under different model specifications.

This markdown is then rendered and uploaded as an artifact. The last component, posting a comment to the PR with the rendered markdown as an object, I am still stuck on. I could try using touchstone? I am not sure why the live-codes version isn't recognising the id of the artifact.

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.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 13, 2025

Great idea to use live-codes! I didn't know of its existence.

I am not sure why the live-codes version isn't recognising the id of the artifact.

I think it might be because it expects the artifact to be uploaded by a different action (for whatever reason) which then triggers the live-codes action - as in the example at https://github.com/live-codes/pr-comment-from-artifact/blob/15cb31a249e2336571f5d8cc244f6132b7d90ead/README.md?plain=1#L29

@jamesmbaazam
Copy link
Contributor

jamesmbaazam commented Jan 13, 2025

Linked issue number should be #917.

@kaitejohnson kaitejohnson changed the title Issue 197: Create a markdown with the results of the synthetic recovery runs Issue 917: Create a markdown with the results of the synthetic recovery runs Jan 13, 2025
@kaitejohnson
Copy link
Contributor Author

kaitejohnson commented Jan 13, 2025

Great idea to use live-codes! I didn't know of its existence.

Clearly I haven't either. I made a separate action since the comment is dependent on the success of the "synthetic validation check" (though they probably can be combined into one, I just wasn't sure).

I am also testing out an alternative since it seems a bit clunky to have to make another directory .livecodes directory and another action just to comment with the artifact!

@seabbs
Copy link
Contributor

seabbs commented Jan 13, 2025

@kaitejohnson
Copy link
Contributor Author

you should be able to do this within one file like this https://github.com/epiforecasts/eval-germany-sp-nowcasting/blob/main/.github/workflows/build-and-publish-documentation.yaml I think

My (very limited) understanding is that it requires a separate action and that's why the current one is failing (per @sbfnk's comment) though I could be misunderstanding.

My attempt at that though, with the separate comment-markdown.yaml isn't working either (again, probably just my limited understanding).

An alternative approach uses a GH action that George wrote for a previous project kaitejohnson#5 (comment) which seems to be working, but not sure if we want to use that.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 14, 2025

My (very limited) understanding is that it requires a separate action and that's why the current one is failing (per @sbfnk's comment) though I could be misunderstanding.

It's how I read the error message but that may have been wrong of course.

My attempt at that though, with the separate comment-markdown.yaml isn't working either (again, probably just my limited understanding).

What went wrong? Looking at the action log for 885ee49 (#925) was the comment action never triggered or is this just not showing because the workflow no longer exists?

@kaitejohnson
Copy link
Contributor Author

What went wrong? Looking at the action log for 885ee49 (#925) was the comment action never triggered or is this just not showing because the workflow no longer exists?

The action still exists but was never triggered. Do you manually add a new action to the required actions for a pull request? I think this is how previous repos worked for me but only an admin could add them.. not sure if this is the same but that might be why the separate action isn't working.

@seabbs
Copy link
Contributor

seabbs commented Jan 15, 2025

. Do you manually add a new action to the required actions for a pull request?

No. this suggests there is an issue with the trigger

@seabbs
Copy link
Contributor

seabbs commented Jan 15, 2025

wait is it just because its not on main? so the trigger isn't quite right

@kaitejohnson
Copy link
Contributor Author

wait is it just because its not on main? so the trigger isn't quite right

Possibly, I'm not sure. I opened a PR for the alternative method which uses what George had set up (though now appears to be failing) #928

@seabbs
Copy link
Contributor

seabbs commented Jan 15, 2025

I don't think we want to depend on CFA internal tooling if we can avoid it .

@kaitejohnson
Copy link
Contributor Author

I had thought the action was just a public action (similar to live codes) but now I am getting an error in the epiforecasts org that I didn't get when running from my repo... kaitejohnson#5

I will try as a temporary test, setting the branches to be this branch to see if I can get the action with live codes to trigger

@kaitejohnson
Copy link
Contributor Author

Yeah I am not sure why its skipping it here kaitejohnson#3 @seabbs

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
4 participants