Skip to content
This repository has been archived by the owner on Jun 13, 2023. It is now read-only.

rgdal dependency breaks automatic tests #38

Open
iantaylor-NOAA opened this issue Mar 15, 2018 · 9 comments
Open

rgdal dependency breaks automatic tests #38

iantaylor-NOAA opened this issue Mar 15, 2018 · 9 comments

Comments

@iantaylor-NOAA
Copy link
Contributor

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 travis-ci/travis-ci#5852 are either

  • add lines to .travis.yml, to get libraries required by this package for linux
  • switch dependency from rgdal to sf (on github at https://github.com/r-spatial/sf)

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Hi @James-Thorson it looks like the attachment from @smormede above (in #38 (comment)) 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
Copy link

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
Copy link
Contributor Author

Thanks @smormede.
In commit c80a7b8, 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
Copy link

@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
Copy link
Contributor Author

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 f988eb6.

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
Copy link

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants