Implementation of paging in function name_usage() #291

damianooldoni · 2018-03-07T18:15:36Z

Description

Inspired by paging functionality in occurrence related functions (e.g. occ.search()), we from INBO have implemented paging in the checklist related function name_usage(). It is in fact quite useful to download all species in a dataset at once. To do this, we tried to mantain the same structure and code style. Default limit (limit = 1000) has been mantained as well.

In order to check the validity of parameter limit as performed in occ_search(), I moved check_vals() to file zzz.R. In this way it can be used by both functions: name_usage() and occ_search().
Moreover, there is a discrepancy among the default values of start parameter between name_usage() and occ_search(). We opted to mantain the default value start = NULL as it is now. Probably it would be better to change it to start = 0 as in occ_*** functions.

Related Issue

This PR has been started and should partly tackle the issue #281
The extension of paging along name_*** can follows after this Pull Request.

Please note that the command:

args <- rgbif_compact(list(offset = start, limit = limit,
                             sourceId = sourceId)

has been changed to:

args <- rgbif_compact(list(offset = start, limit = limit,
                             sourceId = sourceId,
                             datasetKey = datasetKey))

in order to solve issue #290. Otherwise I couldn't perform the final check after syncing my fork to upstream/master.

Example

# Name usage for all taxa in a dataset (set sufficient high limit)
test <- name_usage(datasetKey = "9ff7d317-609b-4c08-bd86-3bc404b77c42", limit = 4000)

returns a list of two: $meta and $data where: test$data has 2676 rows, test$meta$offset = 2000, test$meta$limit = 676 and test$meta$endOfRecords = T

On the other hand, the code here below:

# Name usage for all taxa in a dataset (set sufficient high limit)
test <- name_usage(datasetKey = "9ff7d317-609b-4c08-bd86-3bc404b77c42", limit = 1843)

returns a list of two: $meta and $data where: test$data has 1843 rows, test$meta$offset = 1000, test$meta$limit = 843 and test$meta$endOfRecords = F.

While developing paging I was erroneously thinking that start = NULL one of the reasons why my code did'nt work. I checked it again and it seems that it doesn't affect paging at all! And that's nice.

After testing paging functionality by means of additional file and function name_usage_test, I import the changes into name_usage. I also (tried to) remove any code style changes in order to not interfere with scott coding style.

start = NULL would be removed by rgbif_compact function. It is ok for normal usage but it is a problem while paging: it owuld produce following error: Error in urltools::url_encode(x[[i]]) : Not compatible with STRSXP: [type=NULL]. Moreover, NULL + numeric is not like 0 + numeric.

I use splice function in name_usage from purrr package in order to put all data in chunks together

check_vals, originally used ONLY in occ_search(), is now used in name_usage() too. So, better to move it to file zzz.R in order to not have duplicates.

Update branch from upstream/master before submitting PR for implmentation of paging in name_usage() function.

Add {} within if else. Indentation added as well.

sckott · 2018-03-07T19:19:10Z

thanks! i'll have a look

sckott · 2018-03-08T00:10:32Z

R/name_usage.r

  # each of these args must be length=1
  if (!is.null(rank)) stopifnot(length(rank) == 1)
  if (!is.null(name)) stopifnot(length(name) == 1)
  if (!is.null(language)) stopifnot(length(language) == 1)
  if (!is.null(datasetKey)) stopifnot(length(datasetKey) == 1)

  args <- rgbif_compact(list(offset = start, limit = limit,
-                             sourceId = sourceId))
+                             sourceId = sourceId,


Since now there are no repeated parameters that GBIF allows on this route, can you delete the args <- c(args, rank, datasetKey, name, language) line and put each of name, rank, and language into the rgbif_compact(list(...)) call?

Done. 4fee1b2

And similarly: c51d7c3

sckott · 2018-03-08T00:16:53Z

R/name_usage.r

+      outout[[iter]] <- tt
+    }
+    out <- list()
+    out$results <- purrr::map(outout, ~purrr::splice(.$results))


i don't want to add a new pkg dependency unless there is very good reason to do so. Is the purrrr code doing something like lapply(outout, "[[", "results") ?

Done. e88243a.
i copied your way of merging .results in occ_search. It works perfectly. So, purrr is removed.

sckott · 2018-03-08T00:18:31Z

R/name_usage.r

+        numreturned <- length(tt$results)}
+      sumreturned <- sumreturned + numreturned
+      # if less results than maximum
+      if ((numreturned > 0) & (numreturned < 1000)) {


seems like we want a && here instead of & since it doesn't look like you're looking for a vectorized output?

Done here: 4a7b004

sckott · 2018-03-08T00:25:22Z

tests/testthat/test-name_usage.r

@@ -232,3 +232,42 @@ test_that("fails with more than 1", {
    "length\\(rank\\) == 1 is not TRUE")
 })

+
+# paging
+test_that("paging: class data and meta not modified by paging", {


Thanks much for adding tests! Though can you do tests that don't take as much time as these? Before these new tests the test for this file take about 10 sec, and after take about 30 sec

of course once we have test caching (coming soon via vcr pkg nearly to cran) then the long running time won't matter so much since most of time is in the http request which will be cached

You are right. It takes too much time.
Unfortunately it's impossible to make it shorter because gbif takes chunks of 1000 and takes time for doing it.
I have commented all tests about paging (a6742f9). I added the suggestion to uncomment them after test caching will be implemented via vcr package.

sckott · 2018-03-08T00:29:06Z

feel free to change defaul value of start to 0, if you can update the docs too that would be great

offset is always in args, see line 95.

There is no good reason to introduce purrr package.

numreturned is not a vector. && is better.

Testing paging takes a lot of time. I commented them. Uncomment after introducing test caching via vcr package.

Example was updated in .R file but I forgot devtools::document().

damianooldoni · 2018-03-08T12:57:56Z

Thanks @sckott for your fast answer! Very appreciated.
I replied inline.
About the default start: I changed it to 0 here: bbc2342. Documentation updated as well here: cafa3d5.
Let me know if I need to change something more. Thanks.

sckott · 2018-03-10T07:15:04Z

thanks, looks good.

damianooldoni added 19 commits February 13, 2018 22:18

create test version of name_usage for paging implementation

eca75be

update first version of name_usage with paging

07ea311

imrove code readibility add few comments

479e357

reset NULL as start default value

73cc1b9

While developing paging I was erroneously thinking that start = NULL one of the reasons why my code did'nt work. I checked it again and it seems that it doesn't affect paging at all! And that's nice.

import changes from name_usage_test to name_usage file

07b08e4

After testing paging functionality by means of additional file and function name_usage_test, I import the changes into name_usage. I also (tried to) remove any code style changes in order to not interfere with scott coding style.

add test in test-name_usage

9f7f91c

add purrr in dependencies

359d4c8

I use splice function in name_usage from purrr package in order to put all data in chunks together

typo

274c899

add purrr::

cf319b9

improve testing paging in name_usage() with not-multiple of 1000

6223fb1

improve testing name_usage: class not modified by paging

7af24e9

indentation

ea66e1a

moving help function check_vals() to zzz.r

4a35062

check_vals, originally used ONLY in occ_search(), is now used in name_usage() too. So, better to move it to file zzz.R in order to not have duplicates.

update documentation name_usage

e0e416a

Merge remote-tracking branch 'upstream/master' into paging_name_usage

a51be4a

Update branch from upstream/master before submitting PR for implmentation of paging in name_usage() function.

Add a real datasetkey in example

f78a92f

add curls within if else

e3d3302

Add {} within if else. Indentation added as well.

add datasetKey field in list args

75f7d2e

sckott reviewed Mar 8, 2018

View reviewed changes

damianooldoni added 5 commits March 8, 2018 13:21

remove redundant args <- c(args, ...)

4fee1b2

Remove redundant check of offset

c51d7c3

offset is always in args, see line 95.

remove usage of purrr

e88243a

There is no good reason to introduce purrr package.

no need of vectorized AND

4a7b004

numreturned is not a vector. && is better.

Set default start equal to zero

bbc2342

damianooldoni added 3 commits March 8, 2018 13:30

update start default value in documentation

cafa3d5

Comment tests of paging

93333b1

Testing paging takes a lot of time. I commented them. Uncomment after introducing test caching via vcr package.

update name_usage.Rd about paging

a6742f9

Example was updated in .R file but I forgot devtools::document().

sckott merged commit 90979ab into ropensci:master Mar 10, 2018

sckott added this to the v1.0 milestone Mar 10, 2018

damianooldoni deleted the paging_name_usage branch March 12, 2018 09:31

damianooldoni mentioned this pull request Mar 12, 2018

name_usage ignores datasetKey and uuid - names from backbone returned #290

Closed

damianooldoni mentioned this pull request Mar 27, 2018

Implementation internal paging in name_lookup() #295

Merged

niconoe mentioned this pull request May 19, 2020

Implements pagination gbif/pygbif#63

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implementation of paging in function name_usage() #291

Implementation of paging in function name_usage() #291

Uh oh!

damianooldoni commented Mar 7, 2018

Uh oh!

sckott commented Mar 7, 2018

Uh oh!

sckott Mar 8, 2018

Uh oh!

damianooldoni Mar 8, 2018

Uh oh!

damianooldoni Mar 8, 2018

Uh oh!

sckott Mar 8, 2018

Uh oh!

damianooldoni Mar 8, 2018

Uh oh!

sckott Mar 8, 2018

Uh oh!

damianooldoni Mar 8, 2018

Uh oh!

sckott Mar 8, 2018

Uh oh!

damianooldoni Mar 8, 2018

Uh oh!

sckott commented Mar 8, 2018

Uh oh!

damianooldoni commented Mar 8, 2018

Uh oh!

sckott commented Mar 10, 2018

Uh oh!

Uh oh!

Implementation of paging in function name_usage() #291

Implementation of paging in function name_usage() #291

Uh oh!

Conversation

damianooldoni commented Mar 7, 2018

Description

Related Issue

Example

Uh oh!

sckott commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sckott commented Mar 8, 2018

Uh oh!

damianooldoni commented Mar 8, 2018

Uh oh!

sckott commented Mar 10, 2018

Uh oh!

Uh oh!