-
Notifications
You must be signed in to change notification settings - Fork 62
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
Make GGIR run easier on machines not configured for UK/US #1049
Conversation
@jhmigueles I tested this on a Dutch laptop and have now asked the group in Paris to test it on their computer. However, maybe you want to inform the group you developed the @l-k- Previously I was setting |
…done by GGIR function #1047
243b8d8
to
8529c86
Compare
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.
looks good!
… sensitive in part 3 #1047
@@ -147,6 +147,7 @@ g.part3 = function(metadatadir = c(), f0, f1, myfun = c(), | |||
varlist = c(unclass(lsf.str(envir = asNamespace("GGIR"), all = T)), | |||
"MONITOR", "FORMAT"), | |||
envir = as.environment(asNamespace("GGIR"))) | |||
parallel::clusterEvalQ(cl, Sys.setlocale("LC_TIME", "C")) |
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.
nice! Maybe part 6 too? Or can this definitely never apply to part 6?
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.
Yes, but I have now committed that to a different open PR which comes with many other updates to part6, in that way I the merge will go easier 16b97f0
This PR:
dec_reports
anddec_config
to complementsep_reports
andsep_config
that already existed. I have tested this on Dutch configured laptop and it now works smoothly, fixes sep_report and sep_config not sufficient for working with non-UK/US configured machines? #1048.params_output
is passed on to some of the functions.Sys.setlocale("LC_TIME", "C")
in a more systematic way. For part 2 and 5 it turned out to be necessary to also set this inside the functions that are run in parallel, because somehow R does not pass on this setting to the parallel process, fixes Part 2 report stores some day names in local languages #1047.week_weekend_aggregate.part5
was not used, I have now made sure it is actually used.sep_reports
andsep_config
, were not checked for being a character by check_params, now also fixed.Checklist before merging:
inst/NEWS.Rd
with a user-readable summary. Please, include references to relevant issues or PR discussions.DESCRIPTION
file, if you think you made a significant contribution.