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

Quietly load sf namespace when loading sf objects, for improved printing? #2212

Merged
merged 1 commit into from Aug 11, 2023
Merged

Quietly load sf namespace when loading sf objects, for improved printing? #2212

merged 1 commit into from Aug 11, 2023

Conversation

mikemahoney218
Copy link
Contributor

Apologies for opening this as a PR, rather than as an issue, but I wanted to confirm this would actually work before I brought it up.

Right now, as mentioned in r-lib/pillar#552 , print methods for sf objects inheriting from tibbles is a bit broken if sf is not loaded in the session. This primarily impacts serialized objects, which might be read in via (for example) readRDS() without loading sf. The obvious fix for this is to load sf before loading the object, but it can be hard to identify this from the traceback that's returned.

ggplot2 and, as of today, patchwork handle this by attaching a reference to their package namespace to their objects, so that the serialized object winds up loading the package and therefore the correct print methods -- for instance thomasp85/patchwork@f7fbab5 .

I'm wondering if it would be possible or desirable to do similar for sf objects, so that objects loaded via readRDS will print nicely even if sf is not loaded in the session. This PR includes an example of how that might be possible, if it is desirable.

# save out nc using sf HEAD
remotes::install_github("r-spatial/sf", force = TRUE)
system.file("shape/nc.shp", package = "sf") |>
	sf::read_sf() |> 
	saveRDS("nc_sf_head.rds")

# save out nc using this PR
devtools::install_local()
system.file("shape/nc.shp", package = "sf") |>
	sf::read_sf() |> 
	saveRDS("nc_sf_with_attr.rds")

# Printing is a bit wonky for the non-attribute version:
sf_head <- readRDS("nc_sf_head.rds")
head(sf_head, 1)
#>    AREA PERIMETER CNTY_ CNTY_ID NAME  FIPS FIPSNO CRESS_ID BIR74 SID74 NWBIR74
#> 1 0.114     1.442  1825    1825 Ashe 37009  37009        5  1091     1      10
#>   BIR79 SID79 NWBIR79
#> 1  1364     0      19
#>                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                geometry
#> 1 -81.47276, -81.54084, -81.56198, -81.63306, -81.74107, -81.69828, -81.70280, -81.67000, -81.34530, -81.34754, -81.32478, -81.31332, -81.26624, -81.26284, -81.24069, -81.23989, -81.26424, -81.32899, -81.36137, -81.36569, -81.35413, -81.36745, -81.40639, -81.41233, -81.43104, -81.45289, -81.47276, 36.23436, 36.27251, 36.27359, 36.34069, 36.39178, 36.47178, 36.51934, 36.58965, 36.57286, 36.53791, 36.51368, 36.48070, 36.43721, 36.40504, 36.37942, 36.36536, 36.35241, 36.36350, 36.35316, 36.33905, 36.29972, 36.27870, 36.28505, 36.26729, 36.26072, 36.23959, 36.23436
attr(sf_head, ".sf_namespace")
#> NULL

# The attribute version, however, uses the normal pretty printing:
with_attr <- readRDS("nc_sf_with_attr.rds")
with_attr
#> Simple feature collection with 100 features and 14 fields
#> Geometry type: MULTIPOLYGON
#> Dimension:     XY
#> Bounding box:  xmin: -84.32385 ymin: 33.88199 xmax: -75.45698 ymax: 36.58965
#> Geodetic CRS:  NAD27
#> # A tibble: 100 × 15
#>     AREA PERIMETER CNTY_ CNTY_ID NAME  FIPS  FIPSNO CRESS_ID BIR74 SID74 NWBIR74
#>    <dbl>     <dbl> <dbl>   <dbl> <chr> <chr>  <dbl>    <int> <dbl> <dbl>   <dbl>
#>  1 0.114      1.44  1825    1825 Ashe  37009  37009        5  1091     1      10
#>  2 0.061      1.23  1827    1827 Alle… 37005  37005        3   487     0      10
#>  3 0.143      1.63  1828    1828 Surry 37171  37171       86  3188     5     208
#>  4 0.07       2.97  1831    1831 Curr… 37053  37053       27   508     1     123
#>  5 0.153      2.21  1832    1832 Nort… 37131  37131       66  1421     9    1066
#>  6 0.097      1.67  1833    1833 Hert… 37091  37091       46  1452     7     954
#>  7 0.062      1.55  1834    1834 Camd… 37029  37029       15   286     0     115
#>  8 0.091      1.28  1835    1835 Gates 37073  37073       37   420     0     254
#>  9 0.118      1.42  1836    1836 Warr… 37185  37185       93   968     4     748
#> 10 0.124      1.43  1837    1837 Stok… 37169  37169       85  1612     1     160
#> # ℹ 90 more rows
#> # ℹ 4 more variables: BIR79 <dbl>, SID79 <dbl>, NWBIR79 <dbl>,
#> #   geometry <MULTIPOLYGON [°]>
attr(with_attr, ".sf_namespace")
#> function () 
#> NULL
#> <bytecode: 0x1122f6e10>
#> <environment: namespace:sf>

Created on 2023-08-08 with reprex v2.0.2

@mikemahoney218 mikemahoney218 changed the title Quietly load sf namespace when loading sf objects Quietly load sf namespace when loading sf objects, for improved printing? Aug 8, 2023
@edzer edzer merged commit 0e8ab80 into r-spatial:main Aug 11, 2023
7 checks passed
@edzer
Copy link
Member

edzer commented Aug 11, 2023

Cool trick -- thanks!

edzer added a commit that referenced this pull request Aug 11, 2023
edzer added a commit that referenced this pull request Nov 28, 2023
reverts #2212 because of breaking revdeps
@edzer
Copy link
Member

edzer commented Nov 28, 2023

Reverting this now because of revdep errors, see #2275

@edzer
Copy link
Member

edzer commented Nov 28, 2023

I made this now conditional to Sys.getenv("ADD_SF_NAMESPACE") == "true"; if you want to help make this the default feel free to contact all breaking revdep packages and help them fix their tests.

edzer added a commit that referenced this pull request Nov 28, 2023
@mikemahoney218
Copy link
Contributor Author

mikemahoney218 commented Nov 28, 2023

Am I right in thinking it's just tidytransit that failed due to this? That'd be an easy enough fix, I'm happy to open a PR over there

@edzer
Copy link
Member

edzer commented Nov 28, 2023

Am I right in thinking it's just tidytransit that failed due to this? That'd be an easy enough fix, I'm happy to open a PR over there

Probably not: you'd need a full revdep check with and without the PR, then compare. I use things like this for that purpose, and it only takes like 24 hours or so:

    rm -fr $(REVDEPS)/sf
    mkdir -p $(REVDEPS)/sf
    cp sf_*gz $(REVDEPS)/sf
    (cd $(REVDEPS)/sf; _R_CHECK_FORCE_SUGGESTS_=FALSE R --save -e 'av2=NULL; r <- tools::check_packages_in_dir(".", check_args = "--no-vignettes", reverse = list(which = "most", recursive = FALSE), Ncpus = 4)')
    # then: compare
    # > check_packages_in_dir_changes("sf", "sf_ref", output = TRUE)

@mikemahoney218
Copy link
Contributor Author

mikemahoney218 commented Nov 28, 2023

Sounds good, I'll compare 1361d5c and b978d70 and see what the damage looks like. I'll open an issue if it's a fixable number of packages.

@edzer
Copy link
Member

edzer commented Nov 28, 2023

I knew you would!

@edzer
Copy link
Member

edzer commented Mar 20, 2024

I decided (for now) to revert this change, before users get used to it, because I think it will make #2131 impossible, and that is something I want to explore first (and, on success, will have a larger impact).

edzer added a commit that referenced this pull request Mar 20, 2024
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

2 participants