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

Add run_pipeline() first draft #149

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

Add run_pipeline() first draft #149

wants to merge 9 commits into from

Conversation

Bisaloo
Copy link
Member

@Bisaloo Bisaloo commented Apr 25, 2024

  • Please check if the PR fulfills these requirements
  • I have read the CONTRIBUTING guidelines
  • A new item has been added to NEWS.md
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Checks have been run locally and pass

This is a first proposal for a simple run_pipeline() function. We can iterate on it as long as necessary, with the constraint that we won't touch the reports, or write any epi code here.

Specific points to discuss:

  • function name (lack of harmonization in "report" vs "template" terminology)
  • how to deal with already existing template files (overwrite, use or error?) in out_dir
  • what to preview when multiple reports are created?
  • should we pass a path to the data (saved on the local computer), or a data object directly? This relates to discussions we had with @joshwlambert on integrating godataR to episoap.

Could also be used to solve #51, #88.

TODO:

  • Ensure no file name clash across templates, since they are all copied in the same folder.
  • Ensure renv sandbox is exited after call termination

@Karim-Mane
Copy link
Member

@Bisaloo - this looks good to me. Below is what I think about the points you raised:

how to deal with already existing template files (overwrite, use or error?) in out_dir?

I suggest to compare it with the corresponding template in the package and:

  • overwrite (if contents are different and use the one in the package)
  • use (if contents are the same)

should we pass a path to the data (saved on the local computer), or a data object directly?

I prefer the data object due to the following:

  • the pipeline might not contain the code to transform the input data, from a HIS, to make it suitable for multiple template (severity and transmissibility for example). I think we could let the user handle this and perhaps document the required columns for a specific template.

what to preview when multiple reports are created?

Maybe provide more clarification here if possible. Are you referring to a scenario where severity and transmissibility is executed? or where transmissibility is executed with the different methods?

@Bisaloo
Copy link
Member Author

Bisaloo commented Apr 25, 2024

Are you referring to a scenario where severity and transmissibility is executed

Yes, this. The preview option will open the HTML report in a browser window. Currently it only opens the first report but we could potentially open one tab per report. Not completely if it makes sense though, and not sure which tab should be active then.

@Karim-Mane
Copy link
Member

Are you referring to a scenario where severity and transmissibility is executed

Yes, this. The preview option will open the HTML report in a browser window. Currently it only opens the first report but we could potentially open one tab per report. Not completely if it makes sense though, and not sure which tab should be active then.

Then I think one report with multiple tabs will be better. {reactable} allows for this but I have not yet figured out how not to show the severity tab (which will be empty) if the severity template is not executed.

@Bisaloo
Copy link
Member Author

Bisaloo commented Apr 25, 2024

Then I think one report with multiple tabs will be better. {reactable} allows for this but I have not yet figured out how not to show the severity tab (which will be empty) if the severity template is not executed.

This should be achievable through rmdscaffold. Can it wait until episoap 0.2.0?

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.

2 participants