Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign up[REVIEW]: UTDEventData: An R package to access political event data #1322
Comments
whedon
assigned
alexhanna
Mar 14, 2019
whedon
added
the
review
label
Mar 14, 2019
whedon
referenced this issue
Mar 14, 2019
Closed
[PRE REVIEW]: UTDEventData: An R package to access political event data #1319
This comment has been minimized.
This comment has been minimized.
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @briatte, it looks like you're currently assigned as the reviewer for this paper If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews To fix this do the following two things:
For a list of things I can do to help you, just type:
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Comment # 1 [replicating vignette Quick note re: API registration at http://eventdata.utdallas.edu/signup The UTD server does not check for email address validity and will return an error (500) if provided an invalid email address. That's not something that can be blamed on the package, of course, but the UTD server admins might benefit from improving their API registration form. |
This comment has been minimized.
This comment has been minimized.
Comment # 2 [replicating vignette The error message shown when the API key is wrong could be improved:
|
This comment has been minimized.
This comment has been minimized.
Comment # 3 [replicating vignette Is it deliberate that variable names all start with a space? Example below. This will be confusing for most users.
|
This comment has been minimized.
This comment has been minimized.
Typo in vignette:
|
This comment has been minimized.
This comment has been minimized.
Comment # 4 [replicating vignette The # a boolean logic, or, with the two query blocks
or_query <- orList(list(ctr, time)) It will throw an error if the list is provided in nonstandard-evaluation style:
Perhaps it would help the user if the function would accept objects directly: function (...)
{
return(list(`$or` = list(...))
} Same goes for similar query constructors. |
This comment has been minimized.
This comment has been minimized.
Comment # 5 [replicating vignette Thinking about the API key, it is customary to allow its storage as an environment variable (see |
This comment has been minimized.
This comment has been minimized.
Comment # 6 [replicating vignette The vignette is very helpful (and the authors do a great job at documenting possible errors/traps, e.g. Windows memory issues), yet I believe it would benefit from being broken down in more digestible chunks, such as:
|
This comment has been minimized.
This comment has been minimized.
Comment # 7 [replicating vignette I'm done replicating the vignette. Thinking about the package and data more globally, I think I'd appreciate a mention, somewhere in the docs, of how the data relate to (and whether it can be articulated with) similar event data and related event nomenclatures, e.g. those that Phil Schrodt has worked (or is working) on. Apologies for not being more specific here, as I have limited experience with event data -- if this comment is too vague to be addressed, I'll inquire a bit and reformulate. |
This comment has been minimized.
This comment has been minimized.
Comment # 8 [reviewing Re: this review point,
It seems to me that IEEE papers always come with a DOI. Those are not in the bibliography. Same goes for the Schrodt paper. As for the Althaus et al. dataset (BibTeX reference |
This comment has been minimized.
This comment has been minimized.
Comment # 9 [checking my unchecked review points]
Yup. Checking off.
I do not think this really applies here. Performance in this package boils down (mostly) to API query speed. While replicating the vignette, I found some queries to be slow-ish, but given the size of the data returned by some examples (250,000+ obs.), I'm fine with it. Checking off.
Also checking off this one, given that the package cannot really be tested against the API without providing an API key. Some very basic unit tests could be imagined for small parts of the code: perhaps the authors will want to introduce a few control flow checks (e.g. making sure the argument to
Also checking off this one, as I'm not sure the authors really expect community contributions. The README includes everything else needed to email e.g. bugs or questions. That's pretty much it for me: over to @andrewheiss. Last, for the editor (@alexhanna), we need: @whedon check references (see comment # 8 above for why) |
This comment has been minimized.
This comment has been minimized.
@whedon check references |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Dear @KateHyoung (I hope it's fine to address submitting authors in JOSS review threads?) I posted a bunch of numbered comments in the thread above. I hope that you will find them useful in improving your package, which I found helpful and carefully coded. Thanks for your work, looking forward to discussing things further. All the best~ |
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Mar 21, 2019
Thank you @briatte for taking your time to review our R package and documents. Your comments are very helpful to improve our package. Regarding comment #1, I will contact the server manager at UTD to improve security. For others, I will try my best to reflect your comments on codes and vignette and will discuss if I have questions on your comments. Thank you again! |
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Mar 27, 2019
Dear @briatte and other reviewers I have made some changes corresponding to the comments from @briatte.
Ref. KateHyoung/UTDEventData@a469ff9
I am working on editing the vignette according to the comments on it. Once I have done for this work I will post it soon. |
This comment has been minimized.
This comment has been minimized.
Thanks so much for the comprehensive comments, @briatte, and your responses @KateHyoung. @andrewheiss have you had a chance to look through the submission? |
This comment has been minimized.
This comment has been minimized.
Hi! Yep, I've been traveling way too much and have had unreliable internet access, but I should be able to do the formal review tomorrow. Sorry for the delay! |
This comment has been minimized.
This comment has been minimized.
UTDEventData is a fantastic new package that provides easy access to UT Dallas's event data server. This is an important package, given the complexities involved in pulling event data from half a dozen different sources, and I've already started to use it in my own research. The package is easy to install and use, and I only came across a few issues, noted below. I've opened issues for a few of these at the package repository. DocumentationThe README is fairly sparse and doesn't include any details on how to use the package (beyond providing a list of possible functions). The vignette currently contains complete examples of how to use the package, but reading those examples outside of R (i.e. through GitHub) is trickier since GitHub doesn't display HTML. The README should include instructions about how to set up and use the API key, either as a local variable (e.g. I have not really ever come across instructions to build vignettes when building packages with devtools, even when a package has vignettes (see gganimate, for example), and most packages I've seen on GitHub don't include two different options for package installation (except when one version is on CRAN and one is on GitHub). Additionally, nothing in the vignette is actually run when the package is built, since But there's also utility in actually using vignettes so that users can run There's no right answer here. I don't have any official suggestion for README vs. vignettes, other than the fact that the README should at least include some quick "getting started right away" examples. In a similar vein, the practical examples at the end of the vignette are extremely helpful, but they're buried at the bottom of the long vignette. Perhaps consider splitting the vignette into two parts—one highlighting the different ways of using the different functions, and one with the examples so that users can jump to actual usage faster. API accessAllowing users to store the API key in an environment variable is convenient. It might be worth changing the name of the system variable. Right now it is Additionally, it might be useful (someday later, perhaps—not necessarily for this release) to have the system-level API key a default option in functions like Functionality issuesThe biggest issue is that the S4 class doesn't seem to be working correctly right now. When I run # > obj <- Table$new()
# Error: object 'Table' not found As such, I'm unable to test the rest of the I see the utility of building the API key into the The "Data request function" section doesn't reproduce and there's minimal explanation of what myData <- sendQuery(k,"icews", query_block, citation = TRUE)
# Error in sendQuery(k, "icews", query_block, citation = TRUE) :
# object 'query_block' not found It might be helpful to include code for creating an example There's a comment in the first example that " The call to Clarity and quality of writingThe vignette is generally easy to follow and is a helpful guide to using the package. There are several typos and grammatical errors in the vignette. I've corrected some of the typos in KateHyoung/UTDEventData#9 - a quick round of editing should clean up the rest. TestingThere are no formal tests, likely because the package is heavily reliant on remote access to an API and it would be computationally costly to hit the server for each test. Creating a cache of data locally might be helpful for testing, or perhaps adding tests for non-server-based functions like ContributionsIt might be helpful to have some community guidelines in the package too, such as a CONTRIBUTING.md file (perhaps modeled after something like this or this) and a CONDUCT.md file (like this or this) Phew. That's all I've got. I'm excited for this package! |
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Apr 17, 2019
Dear reviewers, Thank you for your helpful comments for our package. I thoughtfully went over every comment and tried to reflect all of them because those make sense and enhance the package’s operations. For the @briatte ‘s comment #4, we have changed the function code as you suggested. Users now can throw objects directly rather than list up with For the @briatte ‘s comment #7, I have added paragraphs to describe how this package is linked to the other previous political event studies in the introduction part of the vignette. @andrewheiss, thank you for the idea of handling API access in the package. I have learned lots of things from the information you provided me. I have incorporated the way to set an API key in some functions as an environment variable. See the updates in For the comments in Functionality issues, I have fixed the code so that
I have changed/deleted some codes not completely works in R code chunks in the vignette. And the README was restructured and now includes more information for users. Please see here. If you think it needs more information, please let me know. Package’s vignette is edited and restructured according to the comments of @briatte and @andrewheiss. I am trying to list up all my changes from the reviewer’s comments and hope not to miss any important feedback. If I have, please feel free let me know. This entire review process was very useful to make the package stable and functional. I sincerely appreciate your time and comments for this review. Best regards, |
This comment has been minimized.
This comment has been minimized.
@KateHyoung: thanks so much for incorporating these edits. @andrewheiss and @briatte, thanks for the comprehensive comments. Can you two review the changes on the updated package and let me know if you're willing to sign off on the package for meeting the JOSS criteria for acceptance? |
This comment has been minimized.
This comment has been minimized.
These changes look great! I approve and sign off. |
This comment has been minimized.
This comment has been minimized.
The changes look very thorough indeed, and the updated package loads fine, as does the vignette: I'm signing off too. |
This comment has been minimized.
This comment has been minimized.
@whedon accept |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Apr 22, 2019
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Apr 22, 2019
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Apr 22, 2019
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Apr 22, 2019
@whedon generate pdf |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Apr 22, 2019
Dear @danielskatz, I would like to express my gratitude to the editor, @alexhanna, and the reviewers; @briatte and @andrewheiss, again for their times and comments for this R library and its paper. Kate |
This comment has been minimized.
This comment has been minimized.
@whedon accept |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Check final proof If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#636, then you can now move forward with accepting the submission by compiling again with the flag
|
This comment has been minimized.
This comment has been minimized.
Can you also merge KateHyoung/UTDEventData#12 or make similar changes? |
This comment has been minimized.
This comment has been minimized.
KateHyoung
commented
Apr 22, 2019
@danielskatz, I have merged the changes in the bib file. Thank you. |
This comment has been minimized.
This comment has been minimized.
@whedon accept |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Check final proof If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#637, then you can now move forward with accepting the submission by compiling again with the flag
|
This comment has been minimized.
This comment has been minimized.
@whedon accept deposit=true |
This comment has been minimized.
This comment has been minimized.
|
whedon
added
the
accepted
label
Apr 22, 2019
This comment has been minimized.
This comment has been minimized.
Here's what you must now do:
Any issues? notify your editorial technical team... |
danielskatz
closed this
Apr 22, 2019
This comment has been minimized.
This comment has been minimized.
If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
whedon commentedMar 14, 2019
•
edited
Submitting author: @KateHyoung (Hyoungah Kim)
Repository: https://github.com/KateHyoung/UTDEventData
Version: v1.0.0
Editor: @alexhanna
Reviewer: @briatte, @andrewheiss
Archive: 10.5281/zenodo.2648643
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@briatte & @andrewheiss, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @alexhanna know.
Review checklist for @briatte
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?Review checklist for @andrewheiss
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?