ropensci/software-review

submission: parzer

sckott opened this issue · 42 comments

sckott commented

Submitting Author: Scott Chamberlain (@sckott)
Repository: https://github.com/ropenscilabs/parzer
Version submitted: v0.0.4.9110
Editor: @anadiedrichs
Reviewer 1: @mvickm
Reviewer 2: @brunj7
Archive: TBD
Version accepted: TBD

  • Paste the full DESCRIPTION file inside a code block below:
Package: parzer
Type: Package
Title: Parse Coordinates
Description: Parse Coordinates.
Version: 0.0.4.9110
Authors@R: c(
		person("Scott", "Chamberlain", role = c("aut", "cre"),
            email = "sckott@protonmail.com")
	)
License: MIT + file LICENSE
URL: https://github.com/ropenscilabs/parzer
BugReports: https://github.com/ropenscilabs/parzer/issues
Imports:
    Rcpp (>= 1.0.2)
Suggests:
    roxygen2 (>= 6.1.1),
    testthat,
    randgeo,
    knitr,
    rmarkdown
LinkingTo: Rcpp
SystemRequirements: C++11
VignetteBuilder: knitr
Roxygen: list(markdown = TRUE)
Encoding: UTF-8
RoxygenNote: 6.1.1

Scope

  • Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):

    • data retrieval
    • data extraction
    • database access
    • data munging
    • data deposition
    • reproducibility
    • geospatial data
    • text analysis

  • Explain how and why the package falls under these categories (briefly, 1-2 sentences):

parzer fits most closely with the description "manipulating geospatial data" within the geospatial category - b/c it parses many variation of lat/lat coordinates.

  • Who is the target audience and what are scientific applications of this package?

People working with spatial data, either in academia, government, tech, etc.

Not that I know of

  • If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

N/A

Technical checks

Confirm each of the following by checking the box. This package:

Publication options

JOSS Options
  • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)
MEE Options
  • The package is novel and will be of interest to the broad readership of the journal.
  • The manuscript describing the package is no longer than 3000 words.
  • You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
  • (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no guarantee that your manuscript will be within MEE scope.)
  • (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
  • (Please do not submit your package separately to Methods in Ecology and Evolution)

Code of conduct

melvidoni commented

Due to conflicts of interest, I am in the search for an external editor that can take up on this one. Please, be patient with me.

sckott commented

thanks!

melvidoni commented

Hello @sckott! @anadiedrichs will be your editor! Good luck to both of you, and thanks @anadiedrichs for taking up this package review.

anadiedrichs commented

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Hello @sckott , thanks for submitting to rOpenSci. I'll be your handling editor.

The following is the output of goodpractice::gp().

It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 93% of code lines are covered by test cases.

    R/dms-fxns.R:6:NA
    R/parse_lat.R:62:NA
    R/parse_lon.R:67:NA
    R/zzz.R:20:NA

  ✖ fix this R CMD check ERROR: Package suggested but not available:
    ‘randgeo’ The suggested packages are required for a complete check. Checking can be attempted without them by setting the environment variable
    _R_CHECK_FORCE_SUGGESTS_ to a false value. See section ‘The DESCRIPTION file’ in the ‘Writing R Extensions’ manual.

Output of covr::package_coverage()

covr::package_coverage()
parzer Coverage: 93.33%
R/parse_lat.R: 75.00%
R/parse_lon.R: 75.00%
R/zzz.R: 88.89%
R/dms-fxns.R: 96.55%
R/parse_hemisphere.R: 100.00%
R/parse_lat_lon.R: 100.00%
R/parse_parts.R: 100.00%

Please, fix your DESCRIPTION file. Take a look at all of this, and let me know once it is done so that I can check again.

It's my first time as an invited editor. @melvidoni: should the package tests code coverage be 100%? 93 % of coverage is an excellent start point. If it isn't, can I start looking for reviewers if everything is fixed?

  • 👍2
melvidoni commented

Hey @anadiedrichs the % of test is relative to the package, but 93% is great in my personal opinion. Yes, you can start looking for reviewers!

anadiedrichs commented

@sckott: have you already fixed the DESCRIPTION file ?

sckott commented

@anadiedrichs i think it's okay,. correct me if im wrong, but i think that warning is okay b/c you didn't have that package (randgeo) installed on your machine, if you install that pkg and try again that warning should go away

anadiedrichs commented

Hi @sckott, you are right. Consider checking your DESCRIPTION file in case you plan later to submit to CRAN to avoid problems with this issue.
I'm still seeking reviewers.

anadiedrichs commented

We have one confirmed reviewer, Maria Victoria Munafó @mvickm. She's a contributor to the water R package (https://CRAN.R-project.org/package=water) and an active R developer.

I am still looking for the second one.

anadiedrichs commented

We have another reviewer, Julien Brun @brunj7 (http://brunj7.github.io/about/)!! :-)

Thanks for your collaboration @mvickm and @brunj7 to review this issue for rOpenSci.
You have a period of three weeks to review this package from 18th October to 8th November included.

Let me know any questions you have.

njtierney commented

Hi @anadiedrichs can I volunteer to help with this review as a third reviewer?

anadiedrichs commented

Hi Nicholas @njtierney. Of course you can review this issue, but first please make sure you do not have a conflict of interest preventing you from reviewing this package.

njtierney commented

Hi @anadiedrichs - thanks!

I confirm that I do not have a conflict of interest.

njtierney commented

Hi @anadiedrichs - I'm going to step down from reviewing this one, I have realised I have overcommitted myself for November and don't want to drag the review out.

Thanks for letting me help with this, I hope I can help with a review in the near future

anadiedrichs commented

No problem @njtierney, thanks for letting me know.

brunj7 commented

Hi @anadiedrichs and @sckott thank you for the opportunity to review parzer! I have learned a lot from reading @sckott 's code. It is my first review for ropensci, so feel free to ask any questions about my review.

I think parzer is already a solid package and I can see many uses of it to help cleaning some messy data.

My 2 main comments are:

  • I think it will be better if the functions using both lon and lat had their parameters switched so it matches the GIS "convention" of a point defined a c(x=lon, y=lat)
  • I think adding a use case with parzer working with a spatial package could be very valuable to the package documentation

Here is my review:

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be auto-generated via Authors@R).
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:

  • Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Suggestions / comments:

Description of the package

The description of the package could be more specific regarding how it refers to coordinates.

  • Maybe geographic coordinates or latitude and longitude coordinates? I personally prefer the former because the use of geographic could also be referring to geographic vs projected coordinate systems (not covered by parzer by scope definition).

  • Adding more information on how parzer differentiate itself and/or complement sp::char2dms() or biogeo::dms2dd() for example

Documentation

  • Maybe add/switch an example to have using a Southern Hemisphere Lat using S? Just to demonstrate negative numbers transformation.
  • Maybe adding a vignette of a use case of parzer of working with one of the spatial packages? I personally think that will reinforce the statement of need.

Specific functions

  • for the functions using both Latitude and Longitude. By convention, in most of the GIS software/libraries (including sf) coordinates are defined with c(x=lon, y=lat) format. I suggest adopting this format for all the functions of parzer using both latitude and longitude (parse_hemisphere, parse_lat_long). For those same functions, I also suggest in the error handling of out of bound values to ask the user to check if they have not inverted Lon-lat.
  • I am not 100% convinced about the one letter function naming (d(), m(), s())... although I have to admit I am short in suggestions as the name degree is probably used in many packages and won't be necessarily less ambiguous.

Other

  • Not quite sure why goodpractice returned covr test as failed, whereas the unit testing coverage of the package seems to be good. Otherwise, I ran several checks everything returned fine (https://github.com/brunj7/parzer-review/blob/master/jb_checks.R). I have seen some comments about that earlier in this thread.but
covr::package_coverage()
# parzer Coverage: 93.65%
# R/parse_lat.R: 75.00%
# R/parse_lon.R: 75.00%
# R/zzz.R: 88.89%
# src/latlong.cpp: 90.69%
# R/dms-fxns.R: 96.55%
# R/parse_hemisphere.R: 100.00%
# R/parse_lat_lon.R: 100.00%
# R/parse_parts.R: 100.00%
# src/parse.cpp: 100.00%
# src/pz_hemisphere.cpp: 100.00%
# src/pz_parse_parts.cpp: 100.00%
  • Is there a way to reduce Rccp dependencies?
mvickm commented

Hello @anadiedrichs and @sckott , I want to say thank you to consider me for the review as @brunj7 said. Also, it's my first time doing it and I've learn a lot . Thank you again. Here is my review:

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • [ x] A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • [ x] Installation instructions: for the development version of package and any non-standard dependencies in README
  • [ x] Vignette(s) demonstrating major functionality that runs successfully locally
  • [ x] Function Documentation: for all exported functions in R help
  • [ x] Examples for all exported functions in R Help that run successfully locally
  • [ x] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • [x ] Installation: Installation succeeds as documented.

  • [ x] Functionality: Any functional claims of the software been confirmed.

  • [ x] Performance: Any performance claims of the software been confirmed.

  • [x ] Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.

  • [ x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

  • [x ] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Parzer package is a good and useful solution to parse the geographical coordinates in a lot of formats. We can save time using this tool.
Like contributor and user , I think it's necessary to improve the documentation and I suggest the implementation of more study cases.

Is the documentation (installation instructions/vignettes/examples/demos) clear and sufficient?
Maybe could be useful to add at least one example with South Latitude to see the negative numbers.

Were functions and arguments named to work together to form a common, logical programming API that is easy to read, and autocomplete?
The package is coherent in the naming of the variables but it's a little ambivalent.
Final approval (post-review)

[X] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

anadiedrichs commented

Please @mvickm check the box in your comment where says "As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review)." Thanks

anadiedrichs commented

Thanks to @mvickm and @brunj7 for your valuable review. You gave great feedback which can help to improve the quality of the parzer package.
Let us wait & hear the @sckott's answer to chat on how to address the changes.

sckott commented

Sorry for the delay! Preparing responses to reviewers now ...

sckott commented

responses to reviewers (Edited to include all responses): cc @anadiedrichs

reviewer: @brunj7

I think it will be better if the functions using both lon and lat had their parameters switched so it matches the GIS "convention" of a point defined a c(x=lon, y=lat)

I've redone all functions where this applies to make order of parameters: longitude then latitude, see ropensci/parzer#12

adding a use case with parzer working with a spatial package could be very valuable to the package documentation

I added a new vignette of use cases, only with 1 use case for now for spatial packages; see ropensci/parzer#19 - since it reequires additional packages I do not need in parzer, its only available in the pkgdown site: https://docs.ropensci.org/parzer/articles/use_cases.html

Adding more information on how parzer differentiate itself and/or complement sp::char2dms() or biogeo::dms2dd() for example

I've added a section to the README named "Similar art" with notes about how parzer compares to those two functions, see ropensci/parzer#15

Maybe add/switch an example to have using a Southern Hemisphere Lat using S? Just to demonstrate negative numbers transformation.

Added more examples, see ropensci/parzer#14

I also suggest in the error handling of out of bound values to ask the user to check if they have not inverted Lon-lat.

I've updated the warning message from the C code side to also mention that the user should check if they inverted the order, see ropensci/parzer#18

  • I am not 100% convinced about the one letter function naming ...

I'm not either, I've changed these 1 letter functions to have the prefix pz_ so that the addition/subtraction functions are pz_d/pz_m/pz_s and the functions that extract values are pz_degree/pz_minute/pz_second. Does that seem better? See ropensci/parzer#16

  • Maybe geographic coordinates or latitude and longitude coordinates? I personally prefer the former because the use of geographic could also be referring to geographic vs projected coordinate systems (not covered by parzer by scope definition).

Do you mean "latter"? former would be geographic coordinates, correct?

  • Is there a way to reduce Rccp dependencies?

maybe? I'm a bit of a c++ noob, so if someone wants to help me do that i'm happy to do so. Is your main problem with it the extra dependency? opened an issue: ropensci/parzer#17

reviewer: @mvickm

I think it's necessary to improve the documentation and I suggest the implementation of more study cases.

I added a new vignette of use cases, only with 1 use case for now for spatial packages; see ropensci/parzer#19 - since it reequires additional packages I do not need in parzer, its only available in the pkgdown site: https://docs.ropensci.org/parzer/articles/use_cases.html

... add at least one example with South Latitude to see the negative numbers.

Added more examples, see ropensci/parzer#14

The package is coherent in the naming of the variables but it's a little ambivalent.

Can you explain in more detail what you mean by "ambivalent"?

brunj7 commented

Hi @sckott

Thank you for addressing my comments!

To clarify my comment on the short description, I was suggesting to use: "parzer parses messy geographic coordinates". I think it helps to set the scope (vs coordinates alone)

Re: Rccp dependancies -- my comment was motivated by the fact that when I was playing around and reinstalling the package several times at some point the install failed. I could never reproduce the problem and restarting the session solved it. In the aftermath of this, I was wondering if reducing C dependancies could help to avoid any potential problem in the future (I could not reproduce the problem with this new version neither). This being said, my suggestion was more aiming at helping you to think about any ways to simplify dependancies, not at a request to do so. Unfortunately I am not knowledgeable in C and can not help.

@anadiedrichs I have reviewed the new version of parzer and reran all the tests on it. Everything looks good. The package is good to go in my opinion.

  • 👍1
sckott commented

thanks @brunj7

  • Okay, i will fix that language to geographic coordinates
  • Rcpp: okay, thanks for the explanation. there may be something with the Makevars file that I need to fix or so, i'll ask about that
anadiedrichs commented

Thanks, Scott @sckott for addressing the reviewers' comments.
I've ping @mvickm so she can answer your concerns.
Thanks, @brunj7 for your review.

  • 👍1
mvickm commented

HI @sckott ! When I said "ambivalent", I was reffering about the naming of the variables. I mean, for the users maybe could be a little confused using the same name. But I saw you have changed the name and add the prefix _pz. So for me is resolved, Thanks for consider the comments.

  • 👍1
sckott commented

thanks @mvickm for the clarification

@anadiedrichs I need to address some wording changes from @brunj7 comments, and then I will have addressed all comments - will let you know when that's done

  • 👍1
sckott commented

@anadiedrichs Okay, I've fixed the last thing raise about the wording "geographic coordinates". All done on my end

sckott commented

@anadiedrichs Anything else I should do?

  • 👍1
brunj7 commented

Thanks @sckott for the great package and for implementing my last suggestion. @anadiedrichs I realized I had not updated my "Review comment" above by checking the approval box. I just fixed this.

  • 👍2
anadiedrichs commented

Please @mvickm add the following text in your review and check the box if you considered that the package has your final approval.

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.
mvickm commented

@anadiedrichs hi Ana! I already corrected the line in my review comment

anadiedrichs commented

Approved! Thanks, @sckott for submitting and @brunj7 and @mvickm for your reviews! 🥇

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo.
  • Add the rOpenSci footer to the bottom of your README
    " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"
  • Fix all links to the GitHub repo to point to the repo under the ropensci organization.
  • If you already had a pkgdown website, fix its URL to point to https://docs.ropensci.org/package_name and deactivate the automatic deployment you might have set up, since it will not be built centrally like for all rOpenSci packages, see http://devdevguide.netlify.com/#docsropensci. In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
  • Add a mention of the review in DESCRIPTION via rodev::add_ro_desc().
  • Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname).
  • [x ] We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please @brunj7, @mvickm and @sckott tell us what could be improved, the corresponding repo is here.

sckott commented

Thanks very much @anadiedrichs for your work - and thanks to the reviewers @brunj7 and @mvickm

I've done all of those things:

  • transferred from ropenscilabs to ropensci
  • added ropensci footer image
  • fixed all links
  • the docs page is still the ropenscilabs page, but i imagine that will update soon
  • added the review mention in DESCRIPTION
  • no badge links broken as far as I know
  • already have a codemeta.json file

I added one of the reviewers to the DESCRIPTION - @mvickm i don't know your name, and not on your github profile

i'd love to do a tech post if okay with stef.

stefaniebutland commented

i'd love to do a tech post if okay with stef.

🤔 ... yes please!
When do you think you'd have it ready - I can advise on publication date.

  • 👍1
sckott commented

want to get on cran first. Getting it past cran could take a few days or who knows, maybe months, years even (months is possible).

  • 👍1
mvickm commented

Hi @sckott ! I'm glad you've done all the process and was a honour for me be a reviewer. My name is Maria Victoria Munafó and my GitHub profile is @mvickm.

  • 😄1
sckott commented

@mvickm thanks, added

  • 👍1
anadiedrichs commented

I did one last check. All looks good to me. 👍
Anything left to do?

sckott commented

not that I can think of

annakrystalli commented

⚠️⚠️⚠️⚠️⚠️
In the interest of reducing load on reviewers and editors as we manage the
COVID-19 crisis, rOpenSci new submissions for software peer review are paused.

In this period new submissions will not be handled, nor new reviewers assigned.
Reviews and responses to reviews will be handled on a 'best effort' basis, but
no follow-up reminders will be sent.
Other rOpenSci community activities continue.

Please check back here again after 25 May when we will be announcing plans to slowly start back up.

We express our continued great
appreciation for the work of our authors and reviewers. Stay healthy and take
care of one other.

The rOpenSci Editorial Board
⚠️⚠️⚠️⚠️⚠️