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

Issue 486: Fix NOTEs for CRAN submission #514

Merged
merged 6 commits into from
Nov 29, 2023
Merged

Issue 486: Fix NOTEs for CRAN submission #514

merged 6 commits into from
Nov 29, 2023

Conversation

nikosbosse
Copy link
Contributor

@nikosbosse nikosbosse commented Nov 29, 2023

Description

This PR closes #486

During CRAN submission, we got a Note on one of CRAN's machines that CPU time > elapsed time for some examples, tests, and the vignette- this is usually the case when more than 2 cores are used. We had that issue in the past and is almost certainly linked to data.table (see Rdatatable/data.table#3300 and Rdatatable/data.table#5658).

To "solve" this I added the line data.table::setDTthreads(2) before offending code. This is what was suggested in Rdatatable/data.table#5658 by the data.table maintainers.

(The PR also makes a single unrelated linting fix in the vignette)

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2.
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@nikosbosse nikosbosse changed the base branch from main to develop November 29, 2023 09:02
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3db7de1) 90.92% compared to head (e17be30) 90.92%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #514   +/-   ##
=======================================
  Coverage   90.92%   90.92%           
=======================================
  Files          23       23           
  Lines        1399     1399           
=======================================
  Hits         1272     1272           
  Misses        127      127           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nikosbosse nikosbosse changed the base branch from develop to main November 29, 2023 09:08
R/avail_forecasts.R Outdated Show resolved Hide resolved
@@ -279,6 +279,7 @@ check_summary_params <- function(scores,
#' summary is present according to the value specified in `by`.
#' @examples
#' library(magrittr) # pipe operator
#' data.table::setDTthreads(1) # only needed to avoid issues on CRAN
Copy link
Contributor

Choose a reason for hiding this comment

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

why not using the same core count and dontshow everywhere?

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Nearly.

  1. Setting cores to 2 is not what is recommended in that thread but will work and makes sense.
  2. Can we be consistent with setting the number of cores and if we show that to users
  3. Suggest update the comment to be a bit more neutral

vignettes/scoringutils.Rmd Outdated Show resolved Hide resolved
@nikosbosse
Copy link
Contributor Author

this was the recommendation that I used: Rdatatable/data.table#5658 (comment)

Can we be consistent with setting the number of cores and if we show that to users
Suggest update the comment to be a bit more neutral

You're right, I updated the code

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

LGTM

@nikosbosse
Copy link
Contributor Author

🎉
Thank you for reviewing!

@nikosbosse nikosbosse merged commit 413d427 into main Nov 29, 2023
11 checks passed
@nikosbosse nikosbosse deleted the fix-cran branch November 29, 2023 14:07
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.

CRAN release
2 participants