-
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
base: dev
Are you sure you want to change the base?
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!
… applicable in Windows as well in flepimop/main_scripts/inference_main.R
70ef527
to
c5a4433
Compare
Edits mostly for clarity, switched to using `file.path` for platform independence.
@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. |
… 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