search

pfmc-assessments / geostatistical_delta-GLMM

Tool for geostatistical analysis of survey data, for use when estimating an index of abundance
20 stars 17 forks source link

rgdal dependency breaks automatic tests #38

Open iantaylor-NOAA opened 6 years ago

iantaylor-NOAA commented 6 years ago

Automated testing of SpatialDeltaGLMM and [VAST](https://github.com/James-Thorson/VAST) via Travis-CI fail due to error with installation of rgdal. It sounds like the errors are specific to Linux installations, so testthat should still work for any user running it manually on a Windows or Mac computer.

Suggestions at https://github.com/travis-ci/travis-ci/issues/5852 are either

I don't know enough about the use of rgdal (used only by Convert_LL_to_EastNorth_Fn) to know how to judge among these options or make the switch. Perhaps @smormede can help.

smormede commented 6 years ago

Hello,

If you want to use sf rather than rgdal, then attached is a version of Convert_LL_to_EastNorth_Fn that might work. I have tested it outside of the package only so might need to be tested inside the package.

Sophie

(00) projection using sf.txt

James-Thorson commented 6 years ago

Thanks Sophie. I doubt I will have time to explore switching the package for a while, but its good to know what the main alternative would be.

Ian -- if I moved rgdal to ENHANCES from SUGGESTS/DEPENDS do you know if it passes the linux checks on Travis-CI?

iantaylor-NOAA commented 6 years ago

I'm not sure if moving rgdal to ENHANCES would work or not. Travis-CI tests all branches and pull requests, so you could make the change in a new temporary branch and see if it makes a difference.

iantaylor-NOAA commented 6 years ago

Hi @James-Thorson it looks like the attachment from @smormede above (in https://github.com/nwfsc-assess/geostatistical_delta-GLMM/issues/38#issuecomment-376004502) could simply replace the existing Convert_LL_to_EastNorth_Fn function. It would just need a change in the dependencies and testing by somebody with an example that makes use of that function.

If nobody is able to do that testing, why not leave the issue open until it gets done? Otherwise what's the point of having automated tests on Travis-CI for this package or VAST?

smormede commented 6 years ago

Hello. Yes it would just need to swap over the existing function, and I'm happy to test the new function works as expected. I assume @taylori you can test the linux part.

Sophie

iantaylor-NOAA commented 6 years ago

Thanks @smormede. In commit c80a7b80090c5ead3c934e8c69031a6112ee75e4, I added your function in my fork of the repository and changed the DESCRIPTION file to depend on sf instead of rgdal. You can test the complete package with this change by installing this fork via

devtools::install_github('taylori/geostatistical_delta-GLMM')

I'll now create a pull request which should cause Travis-CI to automatically show whether the change fixes the dependency problem that was the source of this issue (it should appear in https://travis-ci.org/nwfsc-assess/geostatistical_delta-GLMM/pull_requests and on the pull request itself). If you give the word, then it seems there shouldn't be any issue with @James-Thorson accepting the pull request and closing this issue for good.

smormede commented 6 years ago

@taylori checks done on Convert_LL_to_EastNorth_Fn via SpatialDeltaGLMM::Prepare_Extrapolation_Data_Fn where Region = "New_Zealand" (which is the only place it gets called I believe). Runs and gives identical results to the previous version. Checked I had the right version of the function. Thanks. Good luck sorting the other issues.

Sophie

iantaylor-NOAA commented 6 years ago

I finally figured out how to get the dependencies working in the automated testing on Travis-CI by making further changes in .travis.yml in commit f988eb610b8448263ffdd8df14fd48700c4e1a52.

However, now that the tests are working, there is an error being reported when running the Chatham Rise test in tests/testthat/test-ChathamRiseHake.R, which seems to be related to the change from rgdal to sf (messages below).

@smormede do you have time to try to see if you can get the Chatham Rise example to work with the sf package? Alternatively, the changes to .travis.yml may be adequate to go back to the second option in my original post of this issue, which is to stick with rgdal but update .travis.yml. I don't know enough about the two packages to know whether one would be a better choice than the other.

Error messages below are from the Travis-CI build of my fork: https://travis-ci.org/taylori/geostatistical_delta-GLMM/builds/359636595

── 1. Error: Chatham Rise hake is working  (@test-ChathamRiseHake.R#19)  ───────
OGR error
1: SpatialDeltaGLMM::Prepare_Extrapolation_Data_Fn(Region = Region, strata.limits = strata.limits) at testthat/test-ChathamRiseHake.R:19
2: SpatialDeltaGLMM::Prepare_NZ_Extrapolation_Data_Fn(strata.limits = strata.limits, 
       ...)
3: SpatialDeltaGLMM::Convert_LL_to_EastNorth_Fn(Lon = Data_Extrap[, "Lon"], Lat = Data_Extrap[, 
       "Lat"], crs = zone)
4: st_transform(dstart, crs)
5: st_transform.sfc(dstart, crs)
6: structure(CPL_transform(x, crs$proj4string), single_type = NULL, crs = crs)
7: CPL_transform(x, crs$proj4string)
smormede commented 6 years ago

Sorry about the delay @taylori . I think SpatialDeltaGLMM::Prepare_NZ_Extrapolation_Data_Fn needs to have zone = 60 as default which should fix the problem. Thanks.