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

dependency on retiring spatial infrastructure packages #89

Closed
rsbivand opened this issue Apr 22, 2023 · 23 comments · Fixed by #95
Closed

dependency on retiring spatial infrastructure packages #89

rsbivand opened this issue Apr 22, 2023 · 23 comments · Fixed by #95
Assignees

Comments

@rsbivand
Copy link

You will be aware, for example from:
https://r-spatial.org/r/2022/04/12/evolution.html,
https://r-spatial.org/r/2022/12/14/evolution2.html,
https://r-spatial.org/r/2023/04/10/evolution3.html and
https://rsbivand.github.io/csds_jan23/bivand_csds_ssg_230117.pdf and
perhaps view https://www.youtube.com/watch?v=TlpjIqTPMCA&list=PLzREt6r1NenmWEidssmLm-VO_YmAh4pq9&index=1
that rgdal, rgeos and maptools will be retired this
year, in October 2023.

maptools is suggested, and in R/fortify-spatial.r maptools::unionSpatialPolygons is used in line 33. Probably this needs revisiting anyway, but https://github.com/r-spatial/evolution/blob/main/pkgapi_230305_refs.csv suggests sf::st_union as an equivalent. The example also uses maptools to read a file, here sf::st_read() would match. It would be helpful to fix this best by June, latest October 2023.

@tdhock
Copy link
Collaborator

tdhock commented May 1, 2023

thanks for the info @Faye-yufan @ampurr this may be addressed over during GSOC. Probably the simplest fix would be to remove this function (I don't think it is used in any examples/tests).

@Faye-yufan
Copy link
Collaborator

Yes for sure!

@ampurr
Copy link
Collaborator

ampurr commented May 2, 2023

Confirming that fortify.sp isn't used in any examples or tests. You're telling people to use broom instead of animint2::fortify anyway, right? Maybe we could just get rid of all the fortify.foo functions while we're at it. (Unless they're used for something, but it doesn't look like it.)

@rsbivand
Copy link
Author

rsbivand commented Jun 1, 2023

Please proceed quickly: with sp set to evolution status 2L, CMD check fails with:
00check.log. Running example(geom_path) finishes for me with:

Warning messages:
1: Removed 1 rows containing missing values (geom_point). 
2: Removed 1 rows containing missing values (geom_path). 
3: Removed 1 rows containing missing values (geom_point). 
4: Removed 1 rows containing missing values (geom_path). 
5: Removed 1 rows containing missing values (geom_point). 

Reproduce with _SP_EVOLUTION_STATUS_=2 R CMD check animint2_2023.3.14.tar.gz. sp with default 2L evolution status will be submitted to CRAN shortly, that is around June 10.

@ampurr
Copy link
Collaborator

ampurr commented Jun 2, 2023

@rsbivand: Thanks for letting us know! But, uh... sorry, I'm confused. I thought we had till October. What's changed?

@rsbivand
Copy link
Author

rsbivand commented Jun 3, 2023

I didn't debug why switching sp's evolution status from 0L to 2L caused CMD check to fail, but it does. Since we need to flip that default status soon, planned CRAN submission June 10, in order to give people with workflows over and above package maintainers time to adapt before October, I' m afraid some change now is needed. You might try keeping evolution status at 0L as a temporary fix, I can help if you like.

@ampurr
Copy link
Collaborator

ampurr commented Jun 3, 2023

Thanks for clarifying and for offering to help. I appreciate it. Unfortunately, as the most technically incompetent member of the animint2 team, I'm unfamiliar with sp. So I don't really know what "evolution" means in this context or why you'd change its status from a 0 to a 2.

@Faye-yufan and @faizan-khan-iit, do you understand what's happening? I also haven't been added as a contributor yet, so I can't do anything anyway.

Skimming through the animint2 repository—"sp" is not an especially useful string to use—it seems like

  • fortify-spatial.r and
  • geom-spoke.r

are the only parts of animint2 that use sp. Is that right? 🤔

@rsbivand
Copy link
Author

rsbivand commented Jun 3, 2023

Only fortify-spatial.r uses maptools, and that was unconditional in code but conditioned in the example. maptools imported sp. The attached as a suggestion removes maptools, removes enhancing sp, adds sf and sp to Suggests:, and uses the shapefile in sf ,and sf to read it, and coerces that object to sp.
animint2-sp.zip

The error I see in geom_path has no link in code to anything here, it must be a dplyr/ggplot2 problem.

@ampurr
Copy link
Collaborator

ampurr commented Jun 3, 2023

Got it, thanks! I'm sure one of my colleagues will implement your solution before the June 10th deadline. 🌈🐱

@rsbivand
Copy link
Author

rsbivand commented Jun 3, 2023

I hope this helps.

@ampurr
Copy link
Collaborator

ampurr commented Jun 3, 2023

For sure. Thanks for taking the time to let us know and come up with solutions. Hope you have a good day and see lots of cats. 😄

@ampurr
Copy link
Collaborator

ampurr commented Jun 7, 2023

Got it, thanks! I'm sure one of my colleagues will implement your solution before the June 10th deadline. 🌈🐱

Fun fact: it turns out that the colleague is me. (Can we be our own colleague? Will the Linguists' Guild come after me if I claim that I am? 🤔)

@rsbivand, I am—and have been—removing the fortify functions instead of using your suggestion for now [1]. I should be finished with that before June 10th.

[1] But, again, your suggestion is appreciated!

@rsbivand
Copy link
Author

rsbivand commented Jun 7, 2023

Great, good luck! fortify was always a problem for spatial data, as Hadley recognised in his useR! keynote at Stanford in 2016 - he then accepted list columns as tidy.

@tdhock
Copy link
Collaborator

tdhock commented Jun 8, 2023

@ampurr can you please write a PR which removes these features, and ask me for review?

@ampurr
Copy link
Collaborator

ampurr commented Jun 8, 2023

Great, good luck! fortify was always a problem for spatial data, as Hadley recognised in his useR! keynote at Stanford in 2016 - he then accepted list columns as tidy.

@rsbivand: Oh, neat! I didn't use R back then, so I wasn't aware of that history. Is that why he tells people to use broom instead? Or does he tell people to use broom for some other reason? 😮

@ampurr can you please write a PR which removes these features, and ask me for review?

@tdhock: Already on it! Will do. 🐈

Some information on what's happening: the removal itself was relatively straightforward, though the non-bijective relationship between the R files and the Rd files made removal somewhat unclear. That part's done. I'm currently debugging some error messages resulting from build.sh and should hopefully be done soon.

@ampurr ampurr self-assigned this Jun 8, 2023
@tdhock
Copy link
Collaborator

tdhock commented Jun 8, 2023

great, thanks, please push what you have so far and create a PR "in progress" then tell me when it is ready for review.

@ampurr ampurr linked a pull request Jun 8, 2023 that will close this issue
@ampurr
Copy link
Collaborator

ampurr commented Jun 9, 2023

@tdhock, @Faye-yufan: Okay, progress update. So, after some experimenting, I'm pretty sure the problem is with the removal of the fortify functions and not with build.sh. Which doesn't solve the bug, but it's useful to know where it comes from.

More specifically, I think the problem lies with layer.r and plot.r, which both use fortify(data), and which I edited. I'm guessing that data relied on fortify() tidying things up.

All right. That's fine. The high priest of R, Hadley Wickham, said to use broom. So I'll just have to steal broom::tidy() and jam it into animint2.

Except! Except there's a small problem: "Data frame tidiers are deprecated and will be removed from an upcoming release of broom." So we're outta luck there and need to figure out an alternate solution.

Anyway, it's real late, so I'm gonna sleep and see what I can figure out when I wake up. Maybe the problem seems more complicated than it actually is. 🐈

@tdhock
Copy link
Collaborator

tdhock commented Jun 11, 2023

I would rather avoid depending on broom, so let's just keep fortify, is that OK with you?

@tdhock
Copy link
Collaborator

tdhock commented Jun 11, 2023

map_data is used in several tests, and that requires fortify.

@ampurr
Copy link
Collaborator

ampurr commented Jun 11, 2023

@tdhock: Of course! It's your package—if it's okay with you, it's okay with me. 🙂

I am curious why ggplot2::fortify() and broom::tidy() are being deprecated. I looked it up and can't find anything. I guess it's not for any super important reason if we're keeping animint2::fortify().

Thanks for solving the problem so quickly and cleanly. I experimented with only removing fortify-spatial.r, but that led to problems. I confess that I, uh... never thought about just editing fortify-spatial.r, which solves the problem nicely.

Thanks also for the note on map_data. Somewhat embarrassingly, my debugging process was not headed in that direction at all. So there may be something wrong with the way I work. I wonder what it is. 🤔

@tdhock
Copy link
Collaborator

tdhock commented Jun 12, 2023

hi @rsbivand I am unable to reproduce the checking examples failure you reported in your uploaded 00check.log file, see below, am I checking correctly?

(base) tdhock@maude-MacBookPro:~$ _SP_EVOLUTION_STATUS_=0 R CMD check ~/R/animint2_2023.3.14.tar.gz
Loading required package: grDevices
* using log directory ‘/home/tdhock/animint2.Rcheck’
* using R version 4.3.0 (2023-04-21)
* using platform: x86_64-pc-linux-gnu (64-bit)
* R was compiled by
    gcc (GCC) 10.1.0
    GNU Fortran (GCC) 10.1.0
* running under: Ubuntu 18.04.6 LTS
* using session charset: UTF-8
* checking for file ‘animint2/DESCRIPTION’ ... OK
* this is package ‘animint2’ version ‘2023.3.14’
* package encoding: UTF-8
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘animint2’ can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking startup messages can be suppressed ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking contents of ‘data’ directory ... OK
* checking data for non-ASCII characters ... OK
* checking LazyData ... OK
* checking data for ASCII and uncompressed saves ... OK
* checking examples ... OK
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘namespace.R’
  Running ‘testthat.R’
 OK
* checking PDF version of manual ... OK
* DONE

Status: OK

(base) tdhock@maude-MacBookPro:~$ _SP_EVOLUTION_STATUS_=2 R CMD check ~/R/animint2_2023.3.14.tar.gz
Loading required package: grDevices
* using log directory ‘/home/tdhock/animint2.Rcheck’
* using R version 4.3.0 (2023-04-21)
* using platform: x86_64-pc-linux-gnu (64-bit)
* R was compiled by
    gcc (GCC) 10.1.0
    GNU Fortran (GCC) 10.1.0
* running under: Ubuntu 18.04.6 LTS
* using session charset: UTF-8
* checking for file ‘animint2/DESCRIPTION’ ... OK
* this is package ‘animint2’ version ‘2023.3.14’
* package encoding: UTF-8
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘animint2’ can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking startup messages can be suppressed ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking contents of ‘data’ directory ... OK
* checking data for non-ASCII characters ... OK
* checking LazyData ... OK
* checking data for ASCII and uncompressed saves ... OK
* checking examples ... OK
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘namespace.R’
  Running ‘testthat.R’
 OK
* checking PDF version of manual ... OK
* DONE

Status: OK

(base) tdhock@maude-MacBookPro:~$ 

Anyway I got that grouped_data error by running the following script, and I fixed in it #95 by re-saving that data set as a plain data.frame (not grouped_data tibble).

library(animint2)
library(dplyr)
     ggplot(economics, aes(date, unemploy)) + geom_line()
     ggplot(economics_long, aes(date, value01, colour = variable)) +
       geom_line()

this was with

> packageVersion("sp")
[1] ‘1.6.0> packageVersion("sf")
[1] ‘1.0.12

@rsbivand
Copy link
Author

I now find this too, for sf 1.0-13 (CRAN), sp 1.6-0, 1.6-1 (CRAN) and 2.0-0 (my fork, branch sp200). I suspect intervening updates in the ggplot2/dplyr or similar (their strong dependencies), though I always update.packages before running rev-dep checks. sp 2.0-0 should be submitted to CRAN this week, to "flip" sp from default status 0 using rgdal under the hood to default status 2 using sf in place of rgdal.

Your branch fix89 has unconnected issues:

* checking for empty or unneeded directories
  NB: this package now depends on R (>= 3.5.0)
  WARNING: Added dependency on R >= 3.5.0 because serialized objects in
  serialize/load version 3 cannot be read in older versions of R.
  File(s) containing such objects:
    'animint2/data/economics_long.rda'
* checking data for non-ASCII characters ... WARNING
  Note: found 10709 marked UTF-8 strings
  Warning: found non-ASCII strings
  'Br<c3><a9>beuf' in object 'montreal.bikes'
  'C<c3><b4>te-Sainte-Catherine' in object 'montreal.bikes'
  'Rachel/H<f4>tel-de-Ville' in object 'montreal.bikes'
  'Ren<e9>-L<e9>vesque_2' in object 'montreal.bikes'
  'Rachel/H<f4>tel de Ville' in object 'montreal.bikes'
  'Ren<e9>-L<e9>vesque' in object 'montreal.bikes'
  '<c0> r<e9>installer' in object 'montreal.bikes'
* checking LazyData ... WARNING
  LazyData DB of 6.6 MB without LazyDataCompression set
  See §1.1.6 of 'Writing R Extensions'
* checking tests ...
  Running ‘namespace.R’
  Running ‘testthat.R’
 ERROR
Running the tests in 'tests/testthat.R' failed.
Last 13 lines of output:
    cannot open the connection
  Calls: local ... eval.parent -> eval -> eval -> eval -> eval -> source -> file
  In addition: Warning message:
  In file(filename, "r", encoding = encoding) :
    cannot open file 'startup.Rs': No such file or directory
  Execution halted
  BEGIN: DOWNLOAD
  BEGIN: POSTDOWNLOAD
  Error in checkError(res) : 
    Undefined error in httr call. httr output: Failed to connect to localhost port 4444 after 0 ms: Couldn't connect to server
  Calls: tests_init -> <Anonymous> -> queryRD -> checkError
  In addition: Warning message:
  In wdman::phantomjs(port = remotePort, phantomver = "latest") :
    PhantomJS development is suspended until further notice: https://github.com/ariya/phantomjs/issues/15344
  Execution halted

but I'm sure you have them under control. There do not seem to be any status 2 problems remaining, also when the retiring packages are not on the library path.

@ampurr
Copy link
Collaborator

ampurr commented Jul 19, 2023

Note to self: Data Visualization: A Practical Introduction has a section on broom in Chapter 6.

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 a pull request may close this issue.

4 participants