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

modified to capture stdout and stderr to log in using system2() being… #289

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

kjsato
Copy link
Contributor

@kjsato kjsato commented Aug 7, 2024

… applicable in Windows as well in flepimop/main_scripts/inference_main.R

Describe your changes.

  • use system2() in place of system(), with dir precheck to capture msgs on both stdout and stdout
  • modified (assumed) to run in Windows as well
    • Checked okay on WSL2-Windows(ubuntu) of course
  • use 'tryCatch'

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

@kjsato kjsato assigned kjsato, saraloo and kimberlynroosa and unassigned kjsato Aug 7, 2024
@kjsato kjsato assigned kjsato and unassigned saraloo and kimberlynroosa Aug 8, 2024
@kjsato
Copy link
Contributor Author

kjsato commented Aug 8, 2024

sorry, I set appropriate persons for reviewers (note that the priority is low)
@kimberlynroosa plz check on MINGW64 environment which you seemed to use

saraloo
saraloo previously approved these changes Sep 11, 2024
Copy link
Contributor

@saraloo saraloo left a 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
Edits mostly for clarity, switched to using `file.path` for platform
independence.
@TimothyWillard
Copy link
Contributor

@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 model_output instead of in a file in the working directory, does this change in behavior sound correct?

@saraloo
Copy link
Contributor

saraloo commented Nov 18, 2024

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

@TimothyWillard
Copy link
Contributor

I think not in the subdirectory for consistency would be better? I'm not sure why this was the choice.

This change has been made.

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.

4 participants