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

Make GGIR run easier on machines not configured for UK/US #1049

Merged
merged 8 commits into from
Feb 21, 2024

Conversation

vincentvanhees
Copy link
Member

@vincentvanhees vincentvanhees commented Feb 19, 2024

This PR:

  • Adds parameters dec_reports and dec_config to complement sep_reports and sep_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.
  • While doing this I also tidied up the way params_output is passed on to some of the functions.
  • Adds 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.
  • I observed that parameter week_weekend_aggregate.part5 was not used, I have now made sure it is actually used.
  • Parameters sep_reports and sep_config, were not checked for being a character by check_params, now also fixed.

Checklist before merging:

  • Existing tests still work (check by running the test suite, e.g. from RStudio).
  • Added tests (if you added functionality) or fixed existing test (if you fixed a bug).
  • Updated or expanded the documentation.
  • Updated release notes in inst/NEWS.Rd with a user-readable summary. Please, include references to relevant issues or PR discussions.
  • Added your name to the contributors lists in the DESCRIPTION file, if you think you made a significant contribution.

@vincentvanhees vincentvanhees marked this pull request as ready for review February 19, 2024 12:13
@vincentvanhees
Copy link
Member Author

vincentvanhees commented Feb 19, 2024

@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 sep_reports argument for to start using these new dec_* parameters and ask them to test it to?

@l-k- Previously I was setting LC_time to C in every specific instance of GGIR code where language is important, with this PR I am attempting to set it more broadly such that we do not always have to worry about language issues. May I ask you to have a look at how I now enforce LC_time. More specifically, I am not sure how to pass on system settings via foreach, so I decided to re-enforce it also inside the subfunctions main_part2 and main_part5 inside g.part2 and g.part5. Does this make sense to you or can you see a more elegant way to do this?

@vincentvanhees vincentvanhees force-pushed the issue1047_force_lang_to_English_part2 branch from 243b8d8 to 8529c86 Compare February 19, 2024 19:28
R/convertEpochData.R Outdated Show resolved Hide resolved
R/g.getstarttime.R Outdated Show resolved Hide resolved
R/GGIR.R Outdated Show resolved Hide resolved
R/g.part2.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@l-k- l-k- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@@ -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"))
Copy link
Collaborator

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?

Copy link
Member Author

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

vincentvanhees added a commit that referenced this pull request Feb 21, 2024
This is part of addressing #1048 and complements PR #1049 but I am adding this in PR #1028 because of all the changes to part 6 in this PR.
@vincentvanhees vincentvanhees removed the request for review from jhmigueles February 21, 2024 07:05
@vincentvanhees vincentvanhees merged commit 7330eb1 into master Feb 21, 2024
10 checks passed
@vincentvanhees vincentvanhees deleted the issue1047_force_lang_to_English_part2 branch February 22, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants