Closed capnrefsmmat closed 1 year ago
capnrefsmmat
commented
1 year ago @brookslogan Do you recall if you were using devtools::check()? I can't figure out how vdiffr determines whether to make plot failures test failures or not, but it hides plot failures on CRAN -- except, evidently, in the additional checks. We might want to manually run the tests on your VM to ensure it's not just hiding the failures.
brookslogan
commented
1 year ago I was primarily using devtools::check() although I tried R CMD check a couple times. At least with devtools::check(), I was getting warnings and failures out of vdiffr consistently; however, they were just the "expected" types (snapshots not existing yet for some things, some plotting style differences on other things) and not the non-closed ring / whatever geometry error they had in the ATLAS checks. Any reason to think it might hide certain vidffr tests and not others? If not, I'd doubt that it is hiding/skipping specifically a geometry-related failure.
brookslogan
commented
1 year ago I tried both with flexiblas and on R-devel using --with-blas="-L/usr/lib64/atlas -lsatlas" --with-lapack. I was never able to find the libRblas.so reference in the notes here. I don't know whether or not this is using LAPACK from R or from ATLAS; perhaps that could be another thing to try out.
bnaras
commented
1 year ago I doubt you need to worry about Atlas failures at least as you describe it; the failures seem to have to do with sf. In case you did not see it, a useful resource to test before submission is winbuilder: https://win-builder.r-project.org. A Mac m1 version to test against is here: https://mac.r-project.org/macbuilder/submit.html. There are also VMs on the rhub project that can be used---you can download the docker files and build your containers---but the cloud versions hang if not enough resources are available or something clogs it up for days. But useful nonetheless when it works. PS. I check against R-devel too.
brookslogan
commented
1 year ago I don't think we're worrying about it, but the CRAN maintainers are apparently worrying about it, which might block any future package updates.
Thanks for the pointers to the builders. I gave rhub local checks a try, but they immediately failed complaining about not setting a CRAN mirror. Haven't tried uploading to R Hub yet, but think @capnrefsmmat might have. I don't think they have a specialized ATLAS container, unfortunately. Not sure if we can get anywhere toward reproducing via R Hub.
capnrefsmmat
commented
1 year ago Yes, I've tested each release against winbuilder, and they always pass. Checks also pass on each machine we have access to. But when we submitted to CRAN, they pointed out the ATLAS failure and said:
please really fix for the next submission.
It's possible there's something different about the machine running the ATLAS check besides ATLAS, but I don't know how to find out what it is.
bnaras
commented
1 year ago I see. @brookslogan Let me try to build a vm per Ripley later today. (I did this a long time ago for something else..)
brookslogan
commented
1 year ago @bnaras many thanks! I ran out of ideas on how to try to reproduce. Would you be compiling from source? Would a Dockerfile starting point be useful?
bnaras
commented
1 year ago Sure, I think the Dockerfile would be useful.
brookslogan
commented
1 year ago Sorry, these aren't the cleanest Dockerfiles. A bit littered with random modifications trying to get things to reproduce.
The Dockerfiles below expect covidcast to be checked out in the same directory as the Dockerfile, in order to do a bind mount to avoid redownloading it a bunch and requiring a GitHub PAT to avoid rate limiting.
bnaras
commented
1 year ago @brookslogan and @capnrefsmmat Here is a Dockerfile that will make it possible to replicate the errors.
capnrefsmmat
commented
1 year ago I've received the official email from Brian Ripley that we must fix this issue by January 9 to keep covidcast on CRAN. I should have some time today to try the Docker image and test my theory for the root cause, and then we can figure out the right solution.
capnrefsmmat
commented
1 year ago I've reproduced the test failures following @bnaras's instructions, so hopefully I can track this down today.
capnrefsmmat
commented
1 year ago Stepping through line-by-line and running sf::st_is_valid() to check the polygons, I've verified the problem comes from the rotation step in shift_pr():
shift_pr <- function(map_df) {
pr_df <- map_df %>% dplyr::filter(.data$is_pr)
pr_df <- sf::st_transform(pr_df, final_crs)
pr_shift <- sf::st_geometry(pr_df) + c(-0.9e+6, 1e+6)
pr_df <- sf::st_set_geometry(pr_df, pr_shift)
r <- 16 * pi / 180
rotation <- matrix(c(cos(r), sin(r), -sin(r), cos(r)), nrow = 2, ncol = 2)
pr_rotate <- (sf::st_geometry(pr_df)) * rotation
pr_df <- sf::st_set_geometry(pr_df, pr_rotate)
# Pretend this was in final_crs all along
suppressWarnings({
sf::st_crs(pr_df) <- final_crs
})
return(pr_df)
}The issue is actually an exception inside GEOS, so I don't think we can suppress our way out of it. I assume that the matrix rotation is somehow resulting in bad polygons because of how ATLAS does the linear algebra. I'll look into alternatives.
capnrefsmmat
commented
1 year ago I submitted the fixed version to CRAN, along with an additional fix to the scales shown on bubble plots. The CRAN maintainers have indicated it's "on its way to CRAN", so we should be good. I'll keep this issue open until the new version is out and I update our pkgdown documentation site.
This is blocking us from releasing updates to the package, including the changes in #598 that improve compatibility with the latest ggplot.
CRAN does checks on standard platforms that ensure all tests run and the package can be built. (Despite the ggplot update, these checks pass on the current package because vdiffr makes plot tests all pass on CRAN, to avoid platform-specific graphical output issues.) It also does "additional checks" on non-standard setups. One of these is a system with ATLAS used as the BLAS implementation; the system setup is documented here.
The package tests and vignettes fail with ATLAS. The output is here. The failures are in sf as it transforms various geometries and, apparently, transforms them into invalid geometries.
Logan set up a VM with ATLAS using flexiblas; see Slack thread. He was unable to reproduce the failures.
If we want to update the package, we'll need to reproduce and diagnose the issue. My hypothesis is that the problem is due to
shift_pr(), which multiplies by a rotation matrix and hence could be affected by BLAS implementation. This is supported by the specific errors in the CRAN output:All of the errors that indicate the geometry in the output give Puerto Rico as the invalid geometry.
But without a way to reproduce the issue, we'll be unable to test this hypothesis, fix the problem, and convince the CRAN maintainers we've fixed it so they'll accept an update.