removed rgdal from data.land and data.remote

[istfer]

Description

Per issue #2914 made changes to data.land. It's a very small change, rgdal functions were being used for printing info only.

I just removed rgdal from data.remote DESCRIPTION as well, although there was an require(rgdal) in the extract_NLCD function, no rgdal package function was being used within this function, also seems to be the case in #3222.

Motivation and Context

See #2914.

Review Time Estimate

• Immediately
• Within one week
• When possible

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation.
• My name is in the list of CITATION.cff
• I have updated the CHANGELOG.md.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[mdietze]

Do we know if raster's dependencies on rgdal have been fixed? If not, this fix could be pretty short lived. I know in other cases we're trying to migrate from raster to terra, but that's a harder fix than the current PR

[istfer]

Ah right, I don't know much about raster, but checking terra some lines can be changed, e.g.:

nlcd <- raster::raster(filename)

to

nlcd <- terra::rast(filename)

and

sites <- sp::spTransform(sites, raster::crs(nlcd))

to

sites <- sp::spTransform(sites, terra::crs(nlcd, proj=TRUE))

then probably instead of raster::extract, terra::extract could be used, but I'm not familiar with this function. Is there someone who can quickly check if such changes would work as intended?

[mdietze]

@istfer Given that this function isn't actively being used in any automated workflow at the moment (i.e. there's no one else who has things set up for a quick and easy test), I'd be OK with you making the obvious changes and leaving a note or warning that they haven't been rigorously tested.

FYI the closest analog is @JoshuaPloshay work on PR #3211 which uses terra to extract a number of other data products.

[istfer]

got it, I made an attempt to replace with terra functions (I found an nlcd image to test it with) and put a warning

[Aariq]

raster no longer imports rgdal. raster has replaced rgdal imports with terra so you do not need to change this (but probably good to do it eventually). If a packages is on CRAN still, it no longer uses the retired r-spatial packages. You can also check if you've gotten rid of all the rgdal dependencies with pak::local_deps_explain("rgdal", "modules/data.land")

[istfer]

I'm guessing CI's API rate limit errors are not something I can fix on my end?

[mdietze]

Nope, you just have to wait for the API to refresh and then try running the ones that failed again

[mdietze]

@istfer the build hit the following errors in check_modules

check modules/data.remote
  2 warnings found in modules/data.remote.
  1 notes found in modules/data.remote.
  Error: Please fix these and resubmit.
  R check of modules/data.remote returned new problems:
  Execution halted
  checking dependencies in R code ... WARNING: '::' or ':::' import not declared from: 'terra'
  checking dependencies in R code ... WARNING: 'library' or 'require' calls not declared from: 'doParallel' 'PEcAn.DB'
  checking dependencies in R code ... WARNING: 'library' or 'require' calls in package code: 'doParallel' 'PEcAn.DB' Please use :: or requireNamespace() instead. See section 'Suggested packages' in the 'Writing R Extensions' manual.

At a minimum, I think you need to remove raster from the DESCRIPTION and add terra. Looks like there are additional issues with 'doParallel' and 'PEcAn.DB' that aren't your fault but may need to be resolved too. For example there's a require(doParallel) in download.thredds.R that we can probably get rid of since the functions appear to be using the namespace. Same with the library(PEcAn.DB) in NLCD.R