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

Resolving issues #17, #34, #42 #44

Merged
merged 57 commits into from
Jun 26, 2024
Merged

Resolving issues #17, #34, #42 #44

merged 57 commits into from
Jun 26, 2024

Conversation

wleoncio
Copy link
Member

C++ code has been confirmed to be faster than R, which should be enough to close issue #17 (and, by extension, #42).

Unit tests have been added for all valid combinations of parallel and pal, which closes #34.

wleoncio added 30 commits April 11, 2024 13:16
This makes it more clear which engine (R or C++) should be used, potentially leading to fewer communications between the languages
It can't be easily suppressed otherwise
Defining `my_values` as `const` (#17)
Removed and de-nested some definitions (#17)
Avoids unnecessary transposition
@wleoncio
Copy link
Member Author

All CodeFactor issues are minor and most are not related to the issues mentioned here, so they should be worked on a different issue.

@wleoncio
Copy link
Member Author

All checks failing on parallel code:

── Error ('test-parallel.R:78:3'): (code run outside of test_that()) ─────────
Error in .check_ncores(nnodes): 6 simultaneous processes spawned

Can reproduce locally, but only by running devtools::check(), as a manual run of the tests through devtools::test() does not show anything. I'm converting this PR to a draft while I investigate.

@wleoncio wleoncio marked this pull request as draft June 25, 2024 15:05
@wleoncio
Copy link
Member Author

The solution might be simple and involve limiting the number of cores to 2 on the examples (source: https://stackoverflow.com/a/50571533/1169233).

wleoncio added 3 commits June 26, 2024 09:48
Windows gives errors regarding lack of support for fork clusters. This is to be eventually fixed separately.
@wleoncio wleoncio marked this pull request as ready for review June 26, 2024 08:42
@wleoncio
Copy link
Member Author

Ready for review! Fixing the parallel test issues was as easy as setting cl = 2 (the GitHub Action and CRAN limits), and I also had to skip the parallel tests on Windows (will explain better on a separate issue).

@Theo-qua Theo-qua merged commit 23530d3 into main Jun 26, 2024
6 of 7 checks passed
@wleoncio wleoncio deleted the issue-17 branch June 26, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants