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

sp_2.0-0 draft changing to status 2L #135

Merged
merged 6 commits into from
Jun 20, 2023
Merged

sp_2.0-0 draft changing to status 2L #135

merged 6 commits into from
Jun 20, 2023

Conversation

rsbivand
Copy link
Contributor

@rsbivand rsbivand commented Jun 7, 2023

This flips default status from 0L to 2L, and adds extra messages for places where code may touch rgeos.

If possible, release after local checks. Rev-dep failures:

"dartR" green-striped-gecko/dartR#604

"dispeRse" jgregoriods/dispeRse#1

"ecospat" ecospat/ecospat#41

"geoviz" neilcharles/geoviz#42

"track2KBA" BirdLifeInternational/track2kba#43

"usmap" pdil/usmap#70

Because of the way it was set up, I can't raise an issue in https://github.com/r-spatial/asdar-book.org to remind us that https://github.com/rsbivand/sf_asdar2ed should be folded into the legacy site between now and October. When we archive the three packages, the active code should be that for sf (with terra tracks for the three chapters in addition).

R/projected.R Outdated
else
!o
} else {
warning("Package sf not available")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-sf case would end up returning a character string (the warning message) rather than a logical as is.projected() should; not sure if this was intended?
Shouldn't the grep-for-longlat-in-p4str fallback be used in the non-sf case as well, maybe returning NA if is.na(p4str) || !nzchar(p4str)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so; I need to check what it returns if anything at all, but warnings are not IIUC returned as strings, they are queued for display depending on the general option setting.

Copy link
Contributor Author

@rsbivand rsbivand Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that today's two commits relieve the problem for the mentioned packages. I'll go ahead and re-run revdeps "most" for sp and the retiring packages - some packages took sp as given if retiring packages were strong dependencies. Will report back with rev-dep outcomes run under _R_CHECK_SUGGESTS_ONLY_=true (the commit message in the second commit should have been warn on missing sf if _SP_STARTUP_MESSAGE_ != "none").

@rsbivand
Copy link
Contributor Author

rsbivand commented Jun 10, 2023

Full revdeps on 2.0-0 draft in this PR; some earlier problems cleared by new publication, remaining issues with _R_CHECK_SUGGESTS_ONLY_=true:

"binaryTimeSeries" bilintoh/binaryTimeSeries#2 resolved with sf listed as Suggests:

"bioRad" adokter/bioRad#545 (CRS wrongly generated under only suggests) resolved with sf listed as Suggests:

"biosurvey" claununez/biosurvey#29 resolved with sf listed as Suggests:

"chronosphere" chronosphere-info/r_client#9 resolved with sf listed as Suggests:

"dispeRse" jgregoriods/dispeRse#1 serious

"ecocomDP" EDIorg/ecocomDP#147 resolved with sf listed as Suggests:

"ecospat" ecospat/ecospat#41 (probably not looking at messages in running vignette from biomod2)

"GEOexplorer" guypwhunt/GEOexplorer#44

"geoviz" neilcharles/geoviz#42, maintainer proposing archiving/orphaning

"mapmisc" email 2023_06_09 with DESCRIPTION resolved with sf listed as Suggests:

"quickPlot" PredictiveEcology/quickPlot#25 resolved in development branch

"rasterList" ecor/rasterList_linkToList#1 resolved with sf listed as Suggests:

"shadow" michaeldorman/shadow#7 resolved with sf listed as Suggests:

"track2KBA" BirdLifeInternational/track2kba#43

"trip" Trackage/trip#52

"usmap" pdil/usmap#70 Fixed in github master

"zoon" zoonproject/zoon#429, maintainer proposing archiving/orphaning

@edzer I suggest moving to CRAN submission of 2.0-0.

R/AAA.R Outdated

smess_func <- function() {
where <- get("startup_message", envir=.spOptions)
Smess <- paste("The legacy packages maptools, rgdal, and rgeos, underpinning the sp package,\nwhich was just ", where, "ed, will retire in October 2023.\nPlease refer to R-spatial evolution reports for details, especially\nhttps://r-spatial.org/r/2023/05/15/evolution4.html.\nThe sp package is now running under evolution status ", get_evolution_status(), "\n", ifelse(get_evolution_status() == 2L, " (status 2 uses the sf package in place of rgdal)\n", ""), sep="")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is an important change in wording. As from sp 1.6-1, loading any package that imports sp outputs a verbose and confusing message about "this package" where sp is meant, e.g.:

R> library(rmapshaper)
The legacy packages maptools, rgdal, and rgeos, underpinning this package
will retire shortly. Please refer to R-spatial evolution reports on
https://r-spatial.org/r/2023/05/15/evolution4.html for details.
This package is now running under evolution status 2 

Copy link
Contributor Author

@rsbivand rsbivand Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current status in this PR is:

> loadNamespace("sp")
The legacy packages maptools, rgdal, and rgeos, underpinning the sp package,
which was just loaded, will retire in October 2023.
Please refer to R-spatial evolution reports for details, especially
https://r-spatial.org/r/2023/05/15/evolution4.html.
It may be desirable to make the sf package available;
package maintainers should consider adding sf to Suggests:.
The sp package is now running under evolution status 2
     (status 2 uses the sf package in place of rgdal)

See also Trackage/trip#52 (comment) for @mdsumner 's helpful nudge. There are now also an environment variable and an option to change the default setting ("load" default, "attach" or "none"). From reactions on twitter, some ecologists see the end of the world coming from retiring packages breaking legacy workflows, hence the noisy start-up message, telling them where to go for information.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The verbose start-up message is certainly useful for when sp is being attached. I am less sure about loading as I guess many sp-importing packages have already adapted to evolution status 2 in which case the message is mostly redundant and rather disturbing (but I guess you want to spread the word).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current rev-deps show about 220 packages failing when retiring packages are not on the library path and _R_CHECK_SUGGESTS_ONLY_=true _R_CHECK_FORCE_SUGGESTS_=FALSE. Many of these have stale declarations from when raster needed rgdal and rgeos in Suggests:, most of these were alerted in December 2022, but haven't responded. Hence the nneed for noise at this point.

The evidential background includes the GDAL barn-raising process, when the representation of coordinate reference systems was changed upstream of us - I see plenty of package code turning off messages, and staying with explicitly defunct representations. So lots of hheads buried securely in sand, I'm afraid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I see. The obvious downside of the "load" default for "sp_startup_message" is that the loading of all sp-importing packages that have already been updated or are otherwise unaffected by the evolution becomes unnecessarily noisy. So I think in the near future, sp should really only speak out when attached.

Copy link
Contributor Author

@rsbivand rsbivand Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. The aim is to track the updating/mitigation rate, and relax the message to default "attach" as soon as there is realistic (for some definition) movement. I think few users update.packages at all frequently, so may well still be on sp 1.6-0 at best. I'm thinking of starting a cron job to pull nightly and CMD check new versions of the 200+ packages on my watch list as they update - any ideas? This does miss new packages, so I'd have to look for new reverse dependencies too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watchlist check running, reporting thrice weekly at: https://github.com/r-spatial/evolution/tree/main/watchlist_output. The input were packages with CMD check errors or warnings with _SP_EVOLUTION_STATUS_=2 R_LIBS=/home/rsb/lib/r_libs _R_CHECK_SUGGESTS_ONLY_=true _R_CHECK_FORCE_SUGGESTS_=FALSE (no retiring packages on library path), initially 223 of 696 packages package_dependencies(packages = c("sp", "maptools", "rgdal", "rgeos"), pdb, which = "most", recursive = FALSE, reverse = TRUE) reduced to 222 by dropping rgeos, the released version of which has had an extra parenthesis in a WKT string since forever and which is now caught by GEOS 3.12.0beta. I included sp because some packages using sp silently assume that rgdal oe rgeos are available.

@edzer edzer merged commit 2abc273 into edzer:main Jun 20, 2023
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 this pull request may close these issues.

None yet

3 participants