Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fetching of greater than pagesize pubs #15

Merged
merged 3 commits into from
Feb 13, 2015

Conversation

jefferis
Copy link
Collaborator

@jefferis jefferis commented Feb 3, 2015

Hi @jkeirstead,

Thanks for the nice package. I've noticed that fetching of >pagesize publications by get_publications seems to be broken (on develop and on master). For example for myself:

nrow(get_publications("cuXoCA8AAAAJ"))
nrow(get_publications('cuXoCA8AAAAJ', pagesize = 50))

both give 20 pubs. Proposed fix in attached.

I also suggest increasing pagesize since this has no substantial penalty for short publication lists but has a significant penalty for long lists. For example:

# a colleague with 187 pubs
system.time(get_publications('G6ufCyYAAAAJ', pagesize = 100))
## restart R session or otherwise clear cache
system.time(get_publications('G6ufCyYAAAAJ', pagesize = 20))

## -> 1.06 vs 3.74 seconds for me

* limit of 20 seems low since we nearly always want _all_
  publications and fetching them in chunks of 20 will be costly for
  more prolific authors
@jkeirstead jkeirstead merged commit 9d9701c into jkeirstead:develop Feb 13, 2015
@jkeirstead
Copy link
Owner

Lovely - thanks!

@jefferis jefferis deleted the fix/fetch_gt_pagesize_pubs branch February 13, 2015 19:31
@jefferis
Copy link
Collaborator Author

jefferis commented Mar 9, 2015

Hi @jkeirstead. Thanks for merging this PR. I was wondering if you would consider merging develop into master or even pushing up to CRAN at some point. Thanks, Greg.

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

Successfully merging this pull request may close these issues.

None yet

2 participants