ropensci / software-review

rOpenSci Software Peer Review.
291 stars 104 forks source link

submission: parzer #341

Closed sckott closed 4 years ago

sckott commented 5 years ago

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


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

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

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

Not that I know of

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](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) 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](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) 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 5 years ago

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 5 years ago

thanks!

melvidoni commented 5 years ago

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

anadiedrichs commented 5 years ago

Editor checks:

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?


melvidoni commented 5 years ago

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 5 years ago

@sckott: have you already fixed the DESCRIPTION file ?

sckott commented 5 years ago

@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 5 years ago

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 5 years ago

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 4 years ago

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 4 years ago

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

anadiedrichs commented 4 years ago

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 4 years ago

Hi @anadiedrichs - thanks!

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

njtierney commented 4 years ago

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 4 years ago

No problem @njtierney, thanks for letting me know.

brunj7 commented 4 years ago

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:

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

Documentation

The package includes all the following forms of documentation:

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

Final approval (post-review)

Estimated hours spent reviewing:


Review Comments

Suggestions / comments:

Description of the package

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

Documentation

Specific functions

Other

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%
mvickm commented 4 years ago

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

Documentation

The package includes all the following forms of documentation:

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


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 4 years ago

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 4 years ago

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 4 years ago

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

sckott commented 4 years ago

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 https://github.com/ropenscilabs/parzer/issues/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 https://github.com/ropenscilabs/parzer/issues/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 https://github.com/ropenscilabs/parzer/issues/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 https://github.com/ropenscilabs/parzer/issues/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 https://github.com/ropenscilabs/parzer/issues/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 https://github.com/ropenscilabs/parzer/issues/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: https://github.com/ropenscilabs/parzer/issues/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 https://github.com/ropenscilabs/parzer/issues/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 https://github.com/ropenscilabs/parzer/issues/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"?

sckott commented 4 years ago

cc @anadiedrichs

brunj7 commented 4 years ago

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.

sckott commented 4 years ago

thanks @brunj7

anadiedrichs commented 4 years ago

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

mvickm commented 4 years ago

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.

sckott commented 4 years ago

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

anadiedrichs commented 4 years ago

ACK @sckott

sckott commented 4 years ago

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

sckott commented 4 years ago

@anadiedrichs Anything else I should do?

brunj7 commented 4 years ago

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.

anadiedrichs commented 4 years ago

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)


mvickm commented 4 years ago

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

anadiedrichs commented 4 years ago

Approved! Thanks, @sckott for submitting and @brunj7 and @mvickm for your reviews! :1st_place_medal:

To-dos:

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 4 years ago

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

I've done all of those things:

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 4 years ago

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.

sckott commented 4 years ago

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).

mvickm commented 4 years ago

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.

sckott commented 4 years ago

@mvickm thanks, added

anadiedrichs commented 4 years ago

I did one last check. All looks good to me. :+1: Anything left to do?

sckott commented 4 years ago

not that I can think of

annakrystalli commented 4 years ago

⚠️⚠️⚠️⚠️⚠️ 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 ⚠️⚠️⚠️⚠️⚠️