Misc fixes found when running package checks on newer releases of R #3237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robkooper
reviewed
Nov 30, 2023
| @@ -30,7 +30,7 @@ mcmc.list2init <- function(dat) { | |||
|
|
|||
| ## define variables | |||
| ic <- list() | |||
| n <- nrow(dat[[1]]) | |||
| nr <- nrow(dat[[1]]) | |||
There was a problem hiding this comment.
surprised this was not found earlier.
There was a problem hiding this comment.
Me too! It looks like the typo ought to make this function return all samples instead of just the last ones, which seems like it ought to be pretty obvious. And yet...!
Needed to avoid being bitten by tidyverse/dbplyr#1240 on R 4.2 image
This comment was marked as outdated.
This comment was marked as outdated.
|
@infotroph there are merge conflicts that need to be resolved in order to pull this one in |
This comment was marked as outdated.
This comment was marked as outdated.
mdietze
approved these changes
Jan 30, 2024
not sure why it just now started complaining about these...
infotroph
force-pushed
the
check-on-newer-R
branch
from
64c8877
to
5647ae8
Compare
13 tasks
R CMD check complains that data directory is large and that json is a type not allowed in data/. Moved ecoregion JSONs to inst/extdata; resaved BADM.csv (2 MB) as BADM.rda (74 KB!)
It is not used in this package or elsewhere, so does not need to be installed with the package (especially since it weighs >2 MB and triggers check warnings about installed package size.)
infotroph
force-pushed
the
check-on-newer-R
branch
from
8472eda
to
2111b1e
Compare
infotroph
force-pushed
the
check-on-newer-R
branch
from
ef3b66f
to
e9eea4a
Compare
infotroph
force-pushed
the
check-on-newer-R
branch
from
e16174c
to
b25b6eb
Compare
infotroph
commented
Feb 19, 2024
| @@ -12,6 +12,6 @@ generalized_plate_model <- function(k, refractive, N) { | |||
| length(N) == 1 | |||
| ) | |||
| RT <- matrix(0, 2101, 2) | |||
| outlist <- .Fortran("gpm", k, refractive, N, RT) | |||
| outlist <- .Fortran("gpm", k, refractive, N, RT, PACKAGE="PEcAnRTM") | |||
There was a problem hiding this comment.
@ashiklom are you still lurking in the PEcAn world? Would it be possible to get your eyes on this commit?
TL;DR: Does this patch look OK to you?
This is a proposed fix for intermittent test failures from RTM that have me puzzled for three reasons:
- Tests are only failing on R 4.2, not on older or newer R versions.
- Failure is traceable to .Fortran calls to
foursailsometimes, but not always, returning NaNs. Repeated calls with the same input fail maybe 2/3 of the time but succeed often enough to make me nervous. - Adding the PACKAGE argument to each .Fortran call seems to fix it, which would seem to imply some kind of namespace collision... but I don't know what other package would contain a
foursailsymbol or why symbol lookup would behave nondeterministically from one invocation to another.
|
All checks are passing, so I'll merge this today unless anyone speaks up before then. Substantive changes since the last review:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Review Time Estimate
Types of changes
Checklist: