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

tmap4: checks for CRAN submission #1014

Open
9 tasks done
mtennekes opened this issue Jan 3, 2025 · 24 comments
Open
9 tasks done

tmap4: checks for CRAN submission #1014

mtennekes opened this issue Jan 3, 2025 · 24 comments

Comments

@mtennekes
Copy link
Member

mtennekes commented Jan 3, 2025

Finally on the verge of the CRAN submission of tmap 4. What is needed?

Should

Nice to have

Could

Everything else

Feel free to add items to these lists. Also to close old issues that became irrelevant.

@olivroy
Copy link
Contributor

olivroy commented Jan 3, 2025

Hi @mtennekes

Just re-ran revdep check... Only pacu remaining. (now fixed)

@mtennekes
Copy link
Member Author

MazamaSpatialPlots and tmaptools (GH version) should be ok now.

Not sure how to test pacu. There seems to be no examples in the docs, just vignettes but not easy to reproduce.

@olivroy

This comment has been minimized.

@mtennekes
Copy link
Member Author

Yes, easy to fix indeed. Will do that this afternoon. Any other rev-dep issues left?

@olivroy
Copy link
Contributor

olivroy commented Jan 8, 2025

Nope, we are all set since we gave MazamaSpatialPlots a warning well in advance and the maintainer said he'll update the package once tmap hits CRAN

mtennekes added a commit that referenced this issue Jan 8, 2025
@mtennekes
Copy link
Member Author

@mtennekes I investigated pacu failures further.

In its code, it calls tmap_options(overlays = NULL, basemaps = NULL). https://github.com/cldossantos/pacu/blob/0d88b9baa26fc959a8ddf34666a2a16491a685d4/R/plot.R#L62

#> Error in `tmap::tmap_options()`
#> ! the following options do not exist: overlays, basemaps.

Some v3 translation should be quite straight-forward? Or tmap should emit only warning saying it is ignored?

Should be fixed:

tmap_options(basemaps = "OpenStreetMap")
#> [v3->v4] `tmap_options()`: use basemap.server instead of basemaps

@olivroy

This comment was marked as resolved.

@mtennekes

This comment was marked as resolved.

@tim-salabim

This comment was marked as resolved.

@sjewo

This comment was marked as resolved.

@Nowosad

This comment was marked as resolved.

@olivroy

This comment was marked as resolved.

@mtennekes
Copy link
Member Author

What do you think @Nowosad @olivroy and others: are we ready for CRAN submission? I'm on holiday next week, so I plan to submit it either tomorrow or in two weeks (27/28 Jan).

I've included some urls to vignettes in the documentation. But the more pepper-spray the better (#979 (comment)).

There are still some (non-critical) shiny issues #1024 left, but don't have time to look into those.

@tim-salabim
Copy link

Are you currently depending on dev leafem?

@mtennekes
Copy link
Member Author

I guess not.... @olivroy do you know that?

@olivroy
Copy link
Contributor

olivroy commented Jan 13, 2025

I think we are indeed ready. @tim-salabim I think some features work better with dev leafem, but not sure to what extent. Personally, I have been working with dev leafem for a few weeks.

@olivroy
Copy link
Contributor

olivroy commented Jan 13, 2025

@mtennekes Re-ran rev-dep check.. pacu still has issues

In their code, they call

suppressMessages(tmap_options(initial.options)

https://github.com/cldossantos/pacu/blob/0d88b9baa26fc959a8ddf34666a2a16491a685d4/R/plot.R#L146-L148

Which yields

Error in `x[[i]]`:
! subscript out of bounds

Possibly that ignoring unkown options would be best in this case.

reprex:

library(pacu)

extd.dir <- system.file("extdata", package = "pacu")
area.of.interest <- sf::st_read(file.path(extd.dir, 'cobs_a_aoi.shp'), quiet = TRUE)

available.images <- readRDS(file.path(extd.dir, 'ds-browse-object.rds'))

out.dir <- extd.dir

s2.files <- list.files(out.dir, '\\.zip', full.names = TRUE)

ndvi.img <- pa_compute_vi(s2.files,
                          vi = 'ndvi', 
                          aoi = area.of.interest,
                          verbose = FALSE)

pa_plot(ndvi.img, main = 'NDVI')

@tim-salabim
Copy link

I think some features work better with dev leafem

which ones?

@mtennekes
Copy link
Member Author

Issue with pacu fixed.

Are there any remaining rev-dep issues?

devtools::check() is OK with cran version of leafem and dev version of tmaptools (which I'll submit now).

If all lights are green, I'll submit tmap tomorrow.

@olivroy
Copy link
Contributor

olivroy commented Jan 13, 2025

All good, thank you!

@mtennekes
Copy link
Member Author

tmaptools 3.2 already on its way to CRAN

@olivroy : to check the package files with devtools::build(), I noticed:

  • There are two screenshots used in the shiny vignette that are included, map/figures/shiny_plot/view.jpg. How can we exclude this from the package files, but still make available for the vignette?
  • The old logo is still there and only used in the tm_logo example. The new logo is not in inst, but in map/figures. Can I replace? As follows:
system.file("img/tmap.png", package="tmap")

with

system.file("man/figures/logo.png", package="tmap")

@olivroy
Copy link
Contributor

olivroy commented Jan 14, 2025

it seems that system.file("help", "figures", "logo.png", package = "tmap") may work?

To hide a particular file, just add to .Rbuildignore

man/figures/shiny_plot/view.jpg

@mtennekes
Copy link
Member Author

Did you also encounter these rev-dep issues @olivroy ? Anything we can do to solve them?

CRAN-pretest-waiting email

Dear maintainer,

package tmap_4.0.tar.gz has been auto-processed. The auto-check found problems when checking the first order strong reverse dependencies.
Please reply-all and explain: Is this expected or do you need to fix anything in your package? If expected, have all maintainers of affected packages been informed well in advance? Are there false positives in our results?

*** Changes to worse in reverse dependencies ***
Debian: https://win-builder.r-project.org/incoming_pretest/tmap_4.0_20250127_172915/reverseDependencies/summary.txt
MazamaSpatialPlots, mapping, spatialrisk

Log dir: https://win-builder.r-project.org/incoming_pretest/tmap_4.0_20250127_172915/
The files will be removed after roughly 7 days.

Pretests:
Windows: https://win-builder.r-project.org/incoming_pretest/tmap_4.0_20250127_172915/Windows/00check.log
Debian: https://win-builder.r-project.org/incoming_pretest/tmap_4.0_20250127_172915/Debian/00check.log

Last published version on CRAN:

CRAN Web: https://cran.r-project.org/package=tmap

Best regards,
CRAN teams' auto-check service
Package check result: OK

Changes to worse in reverse depends:

Package: mapping
Check: R code for possible problems
New result: NOTE
mapping_tmap: warning in tm_credits(srcAuth, size =
options$credits.size, col = options$credits.color, fontface =
options$credits.fontface, position = options$credits.position):
partial argument match of 'col' to 'color'
mapping_tmap_US: warning in tm_credits(srcAuth, size =
options$credits.size, col = options$credits.color, fontface =
options$credits.fontface, position = options$credits.position):
partial argument match of 'col' to 'color'

Package: MazamaSpatialPlots
Check: tests
New result: ERROR
Running ‘testthat.R’ [9s/9s]
Running the tests in ‘tests/testthat.R’ failed.
Complete output:
library(testthat)
library(MazamaSpatialUtils)
Loading required package: sf
Linking to GEOS 3.13.0, GDAL 3.10.1, PROJ 9.5.1; sf_use_s2() is TRUE
library(MazamaSpatialPlots)

test_check("MazamaSpatialPlots")
[ FAIL 6 | WARN 0 | SKIP 0 | PASS 35 ]

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-countyMap.R:62:3'): subsets by stateCode correctly ───────────
length(plottedStates) not equal to nrow(countyList).
1/1 mismatches
[1] 0 - 75 == -75
── Failure ('test-countyMap.R:63:3'): subsets by stateCode correctly ───────────
"WA" %in% plottedStates is not TRUE

actual: FALSE
expected: TRUE
── Failure ('test-countyMap.R:64:3'): subsets by stateCode correctly ───────────
"OR" %in% plottedStates is not TRUE

actual: FALSE
expected: TRUE
── Failure ('test-stateMap.R:59:3'): subsets by stateCode correctly ────────────
length(plottedStates) not equal to length(stateCodeList).
1/1 mismatches
[1] 0 - 2 == -2
── Failure ('test-stateMap.R:60:3'): subsets by stateCode correctly ────────────
"WA" %in% plottedStates is not TRUE

actual: FALSE
expected: TRUE
── Failure ('test-stateMap.R:61:3'): subsets by stateCode correctly ────────────
"OR" %in% plottedStates is not TRUE

actual: FALSE
expected: TRUE

[ FAIL 6 | WARN 0 | SKIP 0 | PASS 35 ]
Error: Test failures
Execution halted

Package: spatialrisk
Check: examples
New result: ERROR
Running examples in ‘spatialrisk-Ex.R’ failed
The error most likely occurred in:

base::assign(".ptime", proc.time(), pos = "CheckExEnv")

Name: choropleth

Title: Create choropleth map

Aliases: choropleth

** Examples

test <- points_to_polygon(nl_provincie, insurance, sum(amount, na.rm = TRUE))
109 points fall not within a polygon.
choropleth(test)

── tmap v3 code detected ───────────────────────────────────────────────────────
[v3->v4] tm_polygons(): instead of style = "fisher", use fill.scale =
tm_scale_intervals().
ℹ Migrate the argument(s) 'style', 'n', 'palette' (rename to 'values') to
'tm_scale_intervals()'
[v3->v4] tm_polygons(): migrate the argument(s) related to the legend of the
visual variable fill namely 'title' to 'fill.legend = tm_legend()'
! tm_scale_bar() is deprecated. Please use tm_scalebar() instead.
choropleth(test, id_name = "areaname", mode = "view")

── tmap v3 code detected ───────────────────────────────────────────────────────
[v3->v4] tm_polygons(): instead of style = "fisher", use fill.scale =
tm_scale_intervals().
ℹ Migrate the argument(s) 'style', 'n', 'palette' (rename to 'values') to
'tm_scale_intervals()'
[v3->v4] tm_polygons(): use fill_alpha instead of alpha.
[v3->v4] tm_polygons(): migrate the argument(s) related to the legend of the
visual variable fill namely 'title' to 'fill.legend = tm_legend()'
Error in tmapLeafletRun(o = list(crs = NA, facet.max = 16, facet.flip = FALSE, :
The package "servr" is required.
Calls: ... step4_plot -> do.call -> tmapLeafletRun ->
Execution halted

@olivroy
Copy link
Contributor

olivroy commented Jan 27, 2025

Hi @mtennekes ! For MazamaSpatialPlots, we notified the author MazamaScience/MazamaSpatialPlots#23. Said he would update when tmap hits CRAN.

For spatialrisk, I didn't see this. we'd need to ask them to suggest servr, or tmap would have to import servr.

For mapping, I saw it, but ignored it. Looks like tm_credits() arguments got changed, hence the partial warning, I think this is safe to ignore? We could

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants