Fixing readGWLdata to work with new data #10

steffilazerte · 2017-11-21T17:21:00Z

No description provided.

- downloads well data from BC gov website - match formatting to previous version

- Alphebatize - Add tidyr - Start directly referring to imports explicity (i.e. dplyr::mutate) - Import "%>%" for entire package

…with get_gwl()

… (otherwise failed on check)

- Add rmarkdown to Suggests for vignettes - Update contents to use get_gwl() - Add details for `which` argument

steffilazerte · 2017-11-28T22:47:54Z

This pull does the following:

adds a new function get_gwl to download well data (including documentation)
adds tests to ensure the working of downstream functions
updates the vignette to use get_gwl
removes most global variables (but see comments on issue Avoid global variables #1)

…undwater.R

steffilazerte · 2017-11-29T15:09:25Z

Also forgot to mention: The only thing missing from the downloaded/formatted data is two additional metadata fields: EMS_ID and Station_Name. Right now they're filled with NA, and I'm not sure where to get the information from without web-scraping the well details (for example, but even this one doesn't have an EMS ID...).

ateucher · 2017-11-29T17:53:34Z

Thanks @steffilazerte! I'll try and have a look today or tomorrow.

ateucher

Hey @steffilazerte - this looks really great! The tests and the working vignette are fantastic!

Just a couple of small points which are open for discussion (comments inline).

The only other thing is that I generally try to avoid pipes in package functions because it makes debugging really difficult - I know the package already had pipes in it, just wondering what you think?

ateucher · 2017-12-01T00:13:26Z

R/readGWLdata.R

+
+  well_avg <- utils::read.csv(text = data[[2]], stringsAsFactors = FALSE)
+  well_avg <- well_avg[, names(well_avg)[names(well_avg) != "year"]]
+  well_avg <- tidyr::spread(well_avg, "type", "Value")


I know they were in the original, but I'm not sure we want the historical mean/max/min attached to every data frame... what about a fourth which option ("hist_avg" or something) that allows downloading just that on its own, and leave it up to the user to join them if they need to?

What about an addition argument, hist_avg = FALSE? I've already coded it in so this way we can just wrap the code that downloads and joins in if(hist_avg) {...}. We can make the argument FALSE by default to minimize unnecessary downloading?

This is a great idea!

ateucher · 2017-12-01T00:13:59Z

R/readGWLdata.R

+                             by = "dummydate")
+
+  ################################
+  # Need station name/location meta information!


So there is a source for well metadata coming in the next couple of weeks, if you're able to hold off then we can add that then?

Absolutely, no hurry. We could still push to master, if you think people may want to use it without the meta data. Or we can wait if you think it could get confusing.

ateucher · 2017-12-01T18:19:21Z

R/gwlZypTest.R

-gwlZypTest <- function(dataframe, wells=NULL, byID, col, method="both") {
+#' @export
+
+gwlZypTest <- function(dataframe, wells = NULL, byID, col, method = "both") {



We should check the existence of columns named in byID and col. I will open an issue.

#12 - probably should look across the board to make sure it's done consistently (for later)

ateucher · 2017-12-01T18:24:28Z

R/monthlyValues.R

+    dplyr::group_by(.data$EMS_ID, .data$Well_Num, 
+                    Year = lubridate::year(.data$Date),
+                    Month = lubridate::month(.data$Date)) %>%
+    dplyr::mutate(Date = dplyr::case_when(length(.data$Well_Num) < 5 ~ 


ateucher · 2017-12-01T18:26:14Z

R/readGWLdata.R

-  if (!inherits(path, "textConnection") && !file.exists(path)) {
-    stop(paste0("The file ", path, "does not exist."))
-  }
+  if(!(which %in% c("all", "recent", "daily"))) stop("type must be either 'all', 'recent', or 'daily'.")


What do you think about using which = c("all", "recent", "daily") in the argument list, and then which <- match.arg(which) to check?

I have to admit it never occurred to me, but yes, that would totally work. The only small thing I don't like is that the error message is:

Error in match.arg(which) : 'arg' should be one of “all”, “recent”, “daily”

Which, to an R newbie, isn't really a clear error message.

But either way, 'type' should be 'which' in the original!

I hope you don't mind, but this inspired me to write a function arg_match that does a better job of this. I implemented it for get_gwl in 65acc50

I definitely don't mind, that's a great function! I like it much better than the original.

I think I'll make use of it in a lot of other places!

ateucher · 2017-12-01T18:27:50Z

R/readGWLdata.R

+  httr::stop_for_status(gwl_data)
+  gwl_data <- httr::content(gwl_data, as = "text", encoding = "UTF-8")
+
+  gwl_avg <- httr::GET(paste0(url, "minMaxMean.csv"))


See question below for whether we should always get the minMaxMean...

ateucher · 2017-12-01T18:37:05Z

R/readGWLdata.R

+
+#' Retrive and format groundwater data from BC Government GWL site
+#' 
+#' Go to <http://www.env.gov.bc.ca/wsd/data_searches/obswell/map/> to find your 


Nice documentation

ateucher · 2017-12-01T18:57:29Z

DESCRIPTION

+    ggplot2 (>= 2.0.0),
+    ggmap (>= 2.6.1),
+    httr (>= 1.3.1),
+    lubridate (>= 1.5.0),
    rgdal (>= 1.1-3),


I'm thinking we should move rgdal and sp to Suggests as they're only used for the one function (utm_dd). Then at the top of the body of utm_dd use:

if (!requireNamespace("rgdal") || !requireNamespace("sp")) { stop("You need the sp and rgdal packages installed to use this function") }

Thoughts?

Definitely, especially as rgdal and sp aren't lightweight packages

steffilazerte · 2017-12-01T20:53:44Z

Commit 225c64e was odd. In order to keep rgdal and sp as suggests and to avoid a check error, I had to manually delete their 'import' lines from the NAMESPACE. But if I reran devtools::document() those import lines would be added back in... possibly a bug in document()?

ateucher · 2017-12-01T23:43:52Z

R/utm_dd.R

-    utm <- SpatialPoints(d[2:3], proj4string=CRS(paste0("+proj=utm +datum=", d[4], " +zone=", d[1])))
-    sp <- spTransform(utm, CRS("+proj=longlat"))  
+    utm <- sp::SpatialPoints(d[2:3], 
+                             proj4string = CRS(paste0("+proj=utm +datum=", d[4], " +zone=", d[1])))


I think we need sp::CRS here and sp::coordinates below

Yup, I missed that. I also missed the @imports that called rgdal and sp, which is why I had trouble with the NAMESPACE. Fixed now in fee8dc9.

steffilazerte added 28 commits November 21, 2017 11:17

Fix typo

5fb2d29

Switch readGWLdata for get_gwl_data

824e72d

- downloads well data from BC gov website - match formatting to previous version

Update to Roxygen 6.0.1

3da00cc

Keep readGWLdata with defunct error message

2681642

Changes to imports

aef9494

- Alphebatize - Add tidyr - Start directly referring to imports explicity (i.e. dplyr::mutate) - Import "%>%" for entire package

Changes to imports

49c33cd

- Alphebatize - Add tidyr - Start directly referring to imports explicity (i.e. dplyr::mutate) - Import "%>%" for entire package

Remove OW from Well_Num

acca57d

Don't set row names on a tibble

47d3300

Add tests to confirm monthlyValues() and gwlMonthlyPlot() not broken …

7be8ff4

…with get_gwl()

Remove library specification (H:/R/win_library) from Rproj Check Args…

65cb4a3

… (otherwise failed on check)

Remove old built vignettes

6afa5c1

Update documentation

7e87b19

Add tests for get_gwl

09b27c8

Test that get_gwl() doesn't break downstream. Fixes #8

db3681e

Create place-holder test files

8b19c9a

Rename for consistency

5e86e47

Use explicit imports and tweak formating

c5772b5

Update vignette (Fixes #9)

3a72801

- Add rmarkdown to Suggests for vignettes - Update contents to use get_gwl() - Add details for `which` argument

Make imports explicit where possible

57f2b47

Tweak formatting

aa740db

Re-write without global variables

e0624cd

Tweak formating, rewrite without global variables

fe86f7b

Use explicit imports and tweak formatting

e751276

Add missing explicit imports, tweak formatting

0c009e4

Fix missing imports and global variables

d194b6a

Formatting

4ab743a

Be explicit regarding type and timezone with tests

ae6e887

Format documentation line lengths

d4b0916

steffilazerte requested a review from ateucher November 28, 2017 22:48

steffilazerte added 3 commits November 29, 2017 08:31

Build vignettes

b4bd45a

Add .data import from rlang and centralize import statements to bcgro…

8787364

…undwater.R

Add explicit dplyr import

78fa206

ateucher suggested changes Dec 1, 2017

View reviewed changes

steffilazerte added 3 commits December 1, 2017 13:33

Suggest sp/rgdal instead of imports

95d332d

Formatting

ffe80eb

Fix Suggests namespace issues?

225c64e

Add arg_match function and use in get_gwl

65acc50

ateucher reviewed Dec 1, 2017

View reviewed changes

Remove sp and rgdal @imports, use explicit function imports

fee8dc9

ateucher approved these changes Dec 2, 2017

View reviewed changes

ateucher merged commit 9f1be71 into master Dec 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing readGWLdata to work with new data #10

Fixing readGWLdata to work with new data #10

steffilazerte commented Nov 21, 2017

steffilazerte commented Nov 28, 2017

steffilazerte commented Nov 29, 2017

ateucher commented Nov 29, 2017

ateucher left a comment

ateucher Dec 1, 2017

steffilazerte Dec 1, 2017

ateucher Dec 1, 2017

ateucher Dec 1, 2017

steffilazerte Dec 1, 2017

ateucher Dec 1, 2017 •

edited

Loading

ateucher Dec 2, 2017

ateucher Dec 1, 2017

ateucher Dec 1, 2017

steffilazerte Dec 1, 2017

ateucher Dec 1, 2017 •

edited

Loading

steffilazerte Dec 1, 2017

ateucher Dec 1, 2017

ateucher Dec 1, 2017

ateucher Dec 1, 2017

ateucher Dec 1, 2017

steffilazerte Dec 1, 2017

steffilazerte commented Dec 1, 2017

ateucher Dec 1, 2017

steffilazerte Dec 2, 2017

ateucher Dec 2, 2017

Fixing readGWLdata to work with new data #10

Fixing readGWLdata to work with new data #10

Conversation

steffilazerte commented Nov 21, 2017

steffilazerte commented Nov 28, 2017

steffilazerte commented Nov 29, 2017

ateucher commented Nov 29, 2017

ateucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ateucher Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ateucher Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffilazerte commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ateucher Dec 1, 2017 •

edited

Loading

ateucher Dec 1, 2017 •

edited

Loading