lingtypology: easy mapping for Lingustic Typology #95
Comments
|
Thank you for your submission @agricolamz Editor checks:
Editor commentsCurrently seeking reviewers. It's a good fit and not overlapping.
It is good practice to
✖ add a "BugReports" field to DESCRIPTION, and point it to
a bug tracker. Many online code hosting services provide bug
trackers for free, https://github.com, https://gitlab.com, etc.
✖ avoid long code lines, it is bad for readability. Also,
many people prefer editor windows that are about 80 characters
wide. Try make your lines shorter than 80 characters
R/aff.lang.R:14:1
R/aff.lang.R:16:1
R/area.lang.R:14:1
R/area.lang.R:16:1
R/country.lang.R:16:1
... and 58 more lines
✖ avoid sapply(), it is not type safe. It might return a
vector, or a list, depending on the input data. Consider using
vapply() instead.
R/aff.lang.R:15:3
R/area.lang.R:15:3
R/country.lang.R:18:10
R/is.glottolog.R:28:5
R/iso.lang.R:15:3
... and 5 more lines
✖ not import packages as a whole, as this can cause name
clashes between the imported packages. Instead, import only the
specific functions you need.
✖ avoid 'T' and 'F', as they are just variables which are
set to the logicals 'TRUE' and 'FALSE' by default, but are not
reserved words and hence can be overwritten by the user. Hence,
one should always use 'TRUE' and 'FALSE' for the logicals.
R/is.glottolog.R:NA:NA
R/map.feature.R:NA:NA
R/map.feature.R:NA:NA
R/map.feature.R:NA:NA
R/map.feature.R:NA:NA
... and 8 more linesReviewers: @timelyportfolio (due: 2017-03-09) @tzakharko (due: 2017-03-13) |
|
Dear @sckott, Thank you for your comment. I hope it will take me two days to sort it out. |
|
thank you |
|
Dear @sckott , I tried testthat multiple times, but all the time it returns linux mint Do you know what should I do? Actually it is the third time, when I try to start use testthat... |
|
@agricolamz can you put what you've tried on a different branch from master so I can try to help |
|
Hi, @sckott! I created the test suit. But I still cannot build a package with my tests since I can't install the last version of testthat. And I cannot make Can you help me with that? Can you see tests and tell me that it is what you've expected from me? |
|
What do you mean you can't build it? I'm not sure what that refers to. Can you show me the errors that happen when trying to install latest Also, in your |
|
Dear @sckott , here is a long version of my answer. I changed |
|
Sorry for delay @agricolamz - was traveling and back now, having a look over this |
|
@agricolamz Let's move forward with review - you have tests now and at least they run on travis and they run fine for me as well - I'll kick off process looking for reviewers I'll help with your machine's setup soon |
|
Dear @timelyportfolio, Thank you for your comments! Since
|
|
Thanks @agricolamz |
|
2nd reviewer assigned: @tzakharko - Thanks for reviewing Taras! |
|
Dear @sckott , @tzakharko and @timelyportfolio. I added an additional arguments and rewrote the main function |
|
just a short message to @timelyportfolio and @tzakharko your reviews are due soon 😸 - thx for reviewing! |
|
@agricolamz working through a full review and very nicely done. First suggestion, add |
|
I felt like the review should go in the I should also add some context to my tests. |
|
@timelyportfolio please put the review here in a new comment. thanks |
linqtypology review
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 4 Review Comments
|
|
One other question, what is the difference between |
|
@tzakharko please get your review in, deadline was about 1 week ago |
Review of package lingtypologyThis review is for the
The package pursues two main goals: on one hand, to offer some basic querying functionality of Glottolog language catalogue, and on the other hand, provide simple CLLD-style interactive mapping functionality leveraging the Leaflet library. In addition to the vanilla Glottolog 2.7 database, the package includes a custom author-edited version of the database (the changes are documented in the package vignette). The Glottolog querying interface consists of a family of functions specialised in a single task (e.g. a function that maps language names to its genealogical affiliation, a function that maps language ISO-code to language names etc.). We liked the thought the author put into making the names of the functions regular and predictable (they all follow the same We were puzzled by the fact, that the author of the package chose not to expose the languoid codes used by the Glottolog database (glottocodes). What is exposed is language name (which, as noted by the author himself, is often ambiguous) as well as the ISO-code (which have a lot of limitations, furthermore, its not clear which version of ISO is used). This makes it impossible to use the package with many modern typological datasets, which usually include glottocode mappings. Also, it needs mentioning that the way the author uses the term The second important area of functionality of the package is creating interactive maps about languages. Here, the package offers a light wrapper over the Leaflet for R package. This wrapper integrates with the Glottolog database for querying language coordinates, but can be also used separately, with user-provided data. In the package documentation, the author uses both the terms "cartography" and "mapping". At the same time, the package lacks important features associated with full-featured cartographic maps (orientation, projection, etc.). It does not seem possible to visualise continuous or ordered data (ratios, counts etc.). Furthermore, we were not able to find an option to display a Pacific-centered map by default, which is a common task for creating maps in linguistic typology. In summary, we believe that the strength of the package lies in quick access to Glottolog as well as the ability to quickly create simple maps, which would make it great for teaching and quick illustrations. However, this comes at a great cost of flexibility. In particular, linguists who work with complex databases will most likely find the features of the package to be too limited and are almost always better off using packages such as General recommendations
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Additional comments
|
|
@sckott @agricolamz Sorry about the delay, we wanted to take a bit more time to write a proper review and also we were two people which meant more coordination effort! |
sckott
added
4/review(s)-in-awaiting-changes
topic:linguistics
and removed
3/reviewer(s)-assigned
labels
|
@timelyportfolio and @tzakharko thanks for your reviews! @tzakharko we'd love to give your other reviewer credit - do they have a github account? |
|
@sckott Sure, its @languageSpaceLabs, sorry for not mentioning it earlier. Thanks! |
|
thanks @tzakharko ! |
|
@tzakharko do you have an estimate of how many hrs the review took? |
1 similar comment
|
@tzakharko do you have an estimate of how many hrs the review took? |
|
@agricolamz Both reviews in - let me know if you have any questions |
|
@sckott Thank you. I've made a lot of changes, so I'll write, when all comments from reviews will be satisfied. Now I'm in a fieldwork, so I'll write at the mid of April. Greetings from Daghestan. |
|
I guess around 6-7 hours in total from both of us.
… On 3 Apr 2017, at 23:44, Scott Chamberlain ***@***.***> wrote:
@tzakharko <https://github.com/tzakharko> do you have an estimate of how many hrs the review took?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#95 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJFn5s18H00jnJR8_SQFBXvt7CDXcKxCks5rsWgzgaJpZM4Lw2nk>.
|
|
thanks @tzakharko |
|
Dear @sckott, @languageSpaceLabs, @tzakharko and @timelyportfolio! I've made a lot of changes: most of them are on CRAN, some of them are not.
As a conclusion, I would like to thank my reviewers. I'm really appreciate time that you've spent on my package. I really happy that I learn new nice ideas that I partially implemented. UPD
|
|
Thanks for all the changes @agricolamz On check i see * checking R code for possible problems ... NOTE
polygon.points: no visible global function definition for ‘sd’
Undefined global functions or variables:
sd
Consider adding
importFrom("stats", "sd")
Please do make changes for the following
@timelyportfolio @tzakharko no need to look over this more, but if you want to chime in on anything else do so soon |
|
Dear @sckott, I did all corrections. The only problem is 80 width code. I've made a lot off changes, but there are still a lot of long lines. |
|
@agricolamz Do get line width down to 80 eventually. Approved! Thanks very much for your submission. Remaining steps:
|
|
Thank you, @sckott! I've made last changes with commit is "80 character width Procrustean bed". I transfered Yes, I'm interested in doing a long-form post at 16 of May, when I will upload new version to CRAN. Are there any instruction, where I should post my .rmd? |
|
Try again now on the repo looks like that appveyor build worked closing now as approved, but can still chat here as needed |
|
Should I fork a repository to my github for creating a GitHub page that everybody used to look? And I asked earlear
|
|
@agricolamz for creating a website for your package have a look at |
|
@maelle I used common html created by vigniette as an GitHub page with an adress https://agricolamz.github.io/lingtypology/. When I transfered the repo to rOpenSci, I did not have such rights (and I couldn't change the repo subtitle and set the Github page), but then @sckott have changed something and I got that rights. So I set the Github page. The problem is actually about an old page. If I type https://github.com/agricolamz/lingtypology Github automatically redirect me to https://github.com/ropensci/lingtypology. But it doesn't work with Github pages... So the old link is broken. And my question was, should I fork my package back, for setting redirection for that link? |
|
Oh I see and sorry I can't help you :-( |
|
@agricolamz for the redirect problem, yeah, sounds like forking might be a way to fix that
Sorry missed this. Instructions here for contributing a post https://github.com/ropensci/roweb/blob/master/.github/CONTRIBUTING.md Let me know if you have any questions. |
|
@agricolamz we'd want to get the post up on May 16th, so get it in day before then at latest |
Summary
What does this package do? (explain in 50 words or less):
Provides R with the Glottolog database http://glottolog.org and some more abilities for purposes of linguistic mapping. The Glottolog database contains the catalogue of languages of the world. This package helps researchers to make a linguistic maps, using philosophy of the Cross-Linguistic Linked Data project http://clld.org/, which allows for while at the same time facilitating uniform access to the data across publications. A tutorial for this package is available on GitHub pages https://agricolamz.github.io/lingtypology/ and package vignette.
Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
lingtypology
Who is the target audience?
Linguists, Linguistic typologists
Are there other R packages that accomplish the same thing? If so, what is different about yours?
No, as far as I know
Requirements
Confirm each of the following by checking the box. This package:
Publication options
Yes
paper.mdwith a high-level description in the package root or ininst/.Detail
Does
R CMD check(ordevtools::check()) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
The text was updated successfully, but these errors were encountered: