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

Reverse dependencies: any issues? #908

Closed
mtennekes opened this issue Jul 28, 2024 · 15 comments
Closed

Reverse dependencies: any issues? #908

mtennekes opened this issue Jul 28, 2024 · 15 comments
Labels

Comments

@mtennekes
Copy link
Member

There are 52 reverse dependencies (all variants: c("Depends", "Imports", "LinkingTo", "Suggests", "Enhances")):

> x = tools::CRAN_package_db()
> y = tools::package_dependencies("tmap", which = "all", recursive = FALSE, reverse = TRUE)
> dput(y$tmap)

c("abmR", "abstr", "bangladesh", "bayesEO", "bdl", "bigDM", "blockCV", 
"CAST", "CopernicusDEM", "CvmortalityMult", "disdat", "edbuildmapr", 
"epm", "eSDM", "findSVI", "geocmeans", "glottospace", "GREENeR", 
"happign", "igr", "LabourMarketAreas", "MainExistingDatasets", 
"mapboxapi", "mapping", "MazamaSpatialPlots", "MultiscaleDTM", 
"ofpetrial", "OpenLand", "pct", "ppcSpatial", "prioriactions", 
"rGhanaCensus", "rnaturalearth", "roads", "rsat", "rstac", "sf", 
"simodels", "sits", "SpatialKDE", "SpatialRDD", "spatialreg", 
"spatialrisk", "spdep", "spnaf", "spNetwork", "stats19", "stplanr", 
"tipsae", "tmaptools", "waterquality", "zonebuilder")

I'll contact each maintainer with the request to check compatibility and to ask if there are any issues and/or questions.

Package maintainer: in case you have questions or found issues, please ask them here.

@jkennedyie
Copy link

Hi. For info I noticed::

  • in tm_shape() my ext param was no longer passed to tmaptools::bb() so I now call bb() explicitly to create a bbox. Added tmaptools to package suggests just in case.
  • the tm_shape() help says bbox param only used if is.main = TRUE, but bbox was used in my case when is.main was default NA.
  • I replaced my tm_polygons() etc fill and alpha parameters with 4.0.0 terminology.
  • typo in tm_shape() help: bbox | Bounding box of he map ... should be the map I guess.

Good luck with the final push

mtennekes added a commit that referenced this issue Jul 29, 2024
@mtennekes
Copy link
Member Author

Thx @jkennedyie see latest commit: I've enabled the passing by of bb() arguments: e.g. tm_shape(World[1,], ext = 2) + tm_polygons(), and also updated the docs.

@mtennekes
Copy link
Member Author

Imho, it would be a good idea to have both syntaxes up- and running like this:

if (packageVersion("tmap") > "3.99") {
  # tmap4 code
  tm_shape(World) + 
    tm_polygons("HPI", 
      fill.scale = tm_scale_intervals(breaks = seq(10, 45, by = 5), values = "brewer.rd_yl_bu"))

} else {
  # tmap3 code
  data(World)
  tm_shape(World) + 
    tm_polygons("HPI", breaks = seq(10, 45, by = 5), palette = "RdYlBu") +
  tm_layout(legend.outside = T)
}

This would make the submission to CRAN easier for all of us.

@jkennedyie
Copy link

Changes in that latest commit working for me - v nice.

I was wondering about best practice during the transition...yep makes sense to me until Suggests >= 4.0.0 will work.

@ailich

This comment was marked as resolved.

mtennekes added a commit that referenced this issue Aug 20, 2024
@mtennekes

This comment was marked as resolved.

@olivroy
Copy link
Contributor

olivroy commented Oct 24, 2024

I ran the revdep check Only 3 packages erroring!

https://github.com/r-tmap/tmap/actions/workflows/recheck.yaml

Common errors

# In mapping package
mappingEU(data = euNuts2, var = "total",
           subset = ~I(nuts0_id == "ES"), facets = "nuts2")
#> Error in if (any(!dtl$sel__) || !q$drop.units) { : 
#>  missing value where TRUE/FALSE needed

# in MazamaSpatialPlots
#> Error in FUN(X[[i]], ...) : object 'projection' not found
#> Calls: <Anonymous> ... lapply -> FUN -> do.call -> <Anonymous> -> tmapShape.sf

Possibly a missing option here?

# Error: processing vignette 'LabourMarketAreas.Rmd' failed with diagnostics:
# unused argument (view.legend.position = c("right", "bottom"))

In rsat, partial matching. Seems pretty harmless

genPlotGIS: warning in tm_facets(ncol = layout[2], nrow = layout[1]):
  partial argument match of 'nrow' to 'nrows'
genPlotGIS: warning in tm_facets(ncol = layout[2], nrow = layout[1]):
  partial argument match of 'ncol' to 'ncols'

Here is the result

------- Check for regressions ------
Changes:
Package: LabourMarketAreas
Check: re-building of vignette outputs
Old result: OK
New result: ERROR

Package: MazamaSpatialPlots
Check: examples
Old result: OK
New result: ERROR

Package: MazamaSpatialPlots
Check: re-building of vignette outputs
Old result: OK
New result: ERROR

Package: MazamaSpatialPlots
Check: tests
Old result: OK
New result: ERROR

Package: mapping
Check: R code for possible problems
Old result: OK
New result: NOTE

Package: mapping
Check: examples
Old result: OK
New result: ERROR

Package: rsat
Check: R code for possible problems
Old result: OK
New result: NOTE
Error: Found potential problems
Execution halted

You can download the logs at the bottom of the page (https://github.com/r-tmap/tmap/actions/runs/11488046296)

mtennekes added a commit that referenced this issue Oct 24, 2024
@mtennekes
Copy link
Member Author

Thx @olivroy! Issues from your chunks 2 and 3 should be fixed now: the view.legend.position argument added (redirecting to legend.position (without message yet), and nrows and ncols renamed to nrow and ncol.

Only need to fix the mapping issues in your first chunk....

@mtennekes
Copy link
Member Author

and nrows and ncols renamed to nrow and ncol.

FYI @Nowosad @rsbivand please check your books as this could cause minor issues. It's about tm_facets (in v3 it also was nrow and ncol, that's why I changed it back)

@Nowosad
Copy link
Member

Nowosad commented Oct 25, 2024

Thanks @mtennekes -- I just looked at each use of tm_facets in geocompr and we consistently use "nrow" and "ncol" there (nrows and ncols were not used).

Update: actually -- ncols was used once for an internal code. (geocompx/geocompr#1143)

@mtennekes
Copy link
Member Author

Cannot reproduce the mapping issue anymore. Perhaps it is fixed already?
MazamaSpatialPlotsissue: see #950

@rsbivand
Copy link

@mtennekes In SDSR, for forthcoming tmap 4, tm_facets_wrap is used, for example tm_facets_wrap(columns = 2, rows = 2), and for tmap < 4 tm_facets(free.scales = FALSE, ncol = 2). I guess that should be OK? The same holds in an spdep vignette.

@mtennekes
Copy link
Member Author

@rsbivand If there are 4 facets, tm_facets_wrap(nrow = 2) should do the job. The arguments columns and rows are used by tm_facets_grid to specify the grouping variables for columns and rows respectively. Similar to the by argument of tm_facets_wrap.

@rsbivand
Copy link

OK, noted. tmap HEAD for now doesn't pass CMD check, so I'll wait before trying if necessary.

@olivroy
Copy link
Contributor

olivroy commented Nov 7, 2024

Looks like we were able to resolve all reverse dependencies issues. Thanks @mtennekes for sorting everything out!

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

No branches or pull requests

6 participants