-
Notifications
You must be signed in to change notification settings - Fork 4
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
modified to capture stdout and stderr to log in using system2() being… #289
Conversation
sorry, I set appropriate persons for reviewers (note that the priority is low) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Koji. This looks fine to me. Worth double checking @kimberlynroosa if this works for you but I think should be fine!
70ef527
to
c5a4433
Compare
@saraloo @kimberlynroosa I'm updating this PR to get it over the finish line since it's been finished. Whenever y'all get a chance could you review/test the changes? Also, I noticed that this PR dumps the log to a subdirectory in |
Thanks tim, I can't test the windows side but it looks right to me. I think not in the subdirectory for consistency would be better? I'm not sure why this was the choice. Fine either way though |
This change has been made. |
flepimop/R_packages/inference/inst/scripts/flepimop-inference-main.R
Outdated
Show resolved
Hide resolved
… applicable in Windows as well in flepimop/main_scripts/inference_main.R
Edits mostly for clarity, switched to using `file.path` for platform independence.
Per an @pearsonca suggestion.
f0a70d2
to
7f9d3f0
Compare
… applicable in Windows as well in flepimop/main_scripts/inference_main.R
Describe your changes.
What does your pull request address? Tag relevant issues.
#169
Mentions of relevant team members.
Please check out whether it works if possible @kimberlynroosa @saraloo