[REVIEW]: osrm: Interface Between R and the OpenStreetMap-Based Routing Service OSRM

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@rCarto@elbeejay<!--end-editor-- Reviewers: @JosiahParry, @mikemahoney218, @wcjochem Archive: 10.5281/zenodo.7228376

Status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/66682b47d145b791fe2e8aa292b84cc2"><img src="https://joss.theoj.org/papers/66682b47d145b791fe2e8aa292b84cc2/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/66682b47d145b791fe2e8aa292b84cc2/status.svg)](https://joss.theoj.org/papers/66682b47d145b791fe2e8aa292b84cc2)

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) by leaving comments 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

@JosiahParry & @mikemahoney218, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @elbeejay know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @JosiahParry

📝 Checklist for @mikemahoney218

📝 Checklist for @wcjochem

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf

editorialbot commented 2 years ago

Software report:

github.com/AlDanial/cloc v 1.88  T=0.02 s (1030.1 files/s, 157397.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               12            140            413            838
Markdown                         3            164              0            667
TeX                              1             68              0            315
JSON                             1              0              0            207
YAML                             2             16              4             60
Rmd                              1             42             65             57
-------------------------------------------------------------------------------
SUM:                            20            430            482           2144
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 2 years ago

Wordcount for paper.md is 525

editorialbot commented 2 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1145/2093973.2094062 is OK
- 10.1111/cobi.13326 is OK
- 10.1080/00131881.2017.1339285 is OK
- 10.1080/23754931.2021.1895875 is OK
- 10.1016/j.simpat.2022.102526 is OK
- 10.14295/transportes.v29i2.2385 is OK
- 10.2373/1864-810X.21-04-05 is OK
- 10.1080/23754931.2018.1519458 is OK
- 10.5604/01.3001.0014.5601 is OK
- 10.1016/j.landurbplan.2018.10.008 is OK
- 10.1108/IJHMA-02-2018-0017 is OK
- 10.3390/ijerph18073813 is OK
- 10.1016/j.trd.2021.102964 is OK
- 10.1038/s41598-022-09919-x is OK
- 10.3390/ijgi8090400 is OK
- 10.1007/s41060-022-00328-x is OK
- 10.1016/j.ajo.2020.08.038 is OK
- 10.1177/23998083211040519 is OK
- 10.1016/j.ufug.2021.127097 is OK
- 10.32614/RJ-2018-053 is OK
- 10.21105/joss.01926 is OK

MISSING DOIs

- None

INVALID DOIs

- None

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

mikemahoney218 commented 2 years ago

Review checklist for @mikemahoney218

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/riatelab/osrm/?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@rCarto) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

elbeejay commented 2 years ago

@mikemahoney218 and @JosiahParry thanks again for agreeing to review this submission to JOSS. We've just switched over from whedon to the new editorialbot, and the review process has changed subtly. Please refer to the post at the top of this issue page for a link to the reviewer guidelines as well as the command for generating your reviewer checklist (@mikemahoney218 wasted no time in generating this :confetti_ball:!) -- please feel free to reach out to me with any questions you may have. Please open issues related to this JOSS review in the osrm repository itself, and "link" them to this review issue by posting this URL (https://github.com/openjournals/joss-reviews/issues/4574) in the text of the issue you open.

Thanks again for reviewing for JOSS, I'm going to tell the editorial bot to send out review reminders in 3 weeks just so we don't collectively let this slip through the cracks.

elbeejay commented 2 years ago

@editorialbot remind @mikemahoney218 in three weeks

editorialbot commented 2 years ago

Reminder set for @mikemahoney218 in three weeks

elbeejay commented 2 years ago

@editorialbot remind @JosiahParry in three weeks

editorialbot commented 2 years ago

Reminder set for @JosiahParry in three weeks

elbeejay commented 2 years ago

@editorialbot add @wcjochem as reviewer

editorialbot commented 2 years ago

@wcjochem added to the reviewers list!

elbeejay commented 2 years ago

@wcjochem thanks again for agreeing to review this submission. Please see the above instructions on how to generate your review checklist and let me know if you have any questions about the process. I'll be asking the editorialbot to send you a reminder in 3 weeks as well just so that things stay on track.

elbeejay commented 2 years ago

@editorialbot remind @wcjochem in three weeks

editorialbot commented 2 years ago

Reminder set for @wcjochem in three weeks

JosiahParry commented 2 years ago

Review checklist for @JosiahParry

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/riatelab/osrm/?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@rCarto) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

wcjochem commented 2 years ago

Review checklist for @wcjochem

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the https://github.com/riatelab/osrm/?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@rCarto) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

elbeejay commented 2 years ago

Hello @wcjochem @JosiahParry @mikemahoney218 ! I just wanted to check-in and make sure the reviews are going okay and that this hasn't fallen off of anyone's radar. Let me know if you have any questions about the process. As a reminder please make review comments as issues in the osrm repository with a link to this review issue so everything stays connected.

Thanks! Jay

editorialbot commented 2 years ago

:wave: @mikemahoney218, please update us on how your review is going (this is an automated reminder).

editorialbot commented 2 years ago

:wave: @JosiahParry, please update us on how your review is going (this is an automated reminder).

editorialbot commented 2 years ago

:wave: @wcjochem, please update us on how your review is going (this is an automated reminder).

mikemahoney218 commented 2 years ago

:wave: @mikemahoney218, please update us on how your review is going (this is an automated reminder).

This is still on my radar :smile: and I'm still intending to have a review for you on the original six week timeline. I'd expect most of my review to come at the end of that window due to upcoming travel plans.

wcjochem commented 2 years ago

👋 @wcjochem, please update us on how your review is going (this is an automated reminder).

Hi @elbeejay , yes, I'm still working on this review and will start updating the issues in the repository. Thanks for the reminder!

elbeejay commented 2 years ago

@JosiahParry pinging you to make sure this review is on your radar! Let me know if you have any questions

JosiahParry commented 2 years ago

@JosiahParry pinging you to make sure this review is on your radar! Let me know if you have any questions

Yup, it is :) thanks!

elbeejay commented 2 years ago

@JosiahParry, @wcjochem, @mikemahoney218 it is a new week so I'm just checking in here to make sure the reviews are going smoothly. As always, please reach out if you have any questions about the process.

elbeejay commented 2 years ago

:wave: @JosiahParry, @wcjochem, @mikemahoney218 as we are now in week 5 of the review process, I just wanted to remind all of you that we ask our reviewers to complete their reviews in 6 weeks (September 4 in this case). If you are going to be unable to complete your review of this submission by that date, please notify us in this thread with your estimated date of completion for the benefit of both @rCarto and myself.

Thanks!

mikemahoney218 commented 2 years ago

Thanks for inviting me to review this package @elbeejay and @rCarto! It's a very cool package and I'm happy to have had a reason to get "under the hood".

I've opened two issues on the osrm repository flagging a few sections of the checklist I don't feel quite ready to check off yet: https://github.com/riatelab/osrm/issues/92 https://github.com/riatelab/osrm/issues/93

Other than those issues, everything looks good on my end. Thanks again for having me review, it's really a cool project :smile:

JosiahParry commented 2 years ago

I have reviewed this package and at this moment am unable to check off the sections Substantial Scholarly Effort, Functionality Documentation, Automated Test, Community Guidelines, and Quality of the writing.

elbeejay commented 2 years ago

Thank you to both @mikemahoney218 and @JosiahParry for reviewing the package and opening up issues for each of your comments and suggestions.

After looking through the review issues, I found them all to be reasonable and valid, and encourage @rCarto to make the changes suggested. With regards to https://github.com/riatelab/osrm/issues/102, @JosiahParry I appreciate you raising some concerns about the size and completeness of the package. When osrm was submitted to JOSS it was re-circulated among the editorial team to see whether or not it was within scope during the pre-review phase as it was a "borderline" package in terms of size and scope. Due to its popularity and use across a number of different publications in different fields, we decided that the package would be considered "in-scope." However your point about feature completeness is very well taken -- @rCarto, I agree with @JosiahParry that it is very important for osrm to be fully feature complete with proper tests, input type-checking, and clear documentation with regards to expected input types, assumptions, and internal data transformations.

wcjochem commented 2 years ago

Thank you @elbeejay for the opportunity to review this package.

I echo many of the questions/comments raised by @JosiahParry and @mikemahoney218. Therefore I have added only a few additional issues to the repository which were not covered by the other reviewers: riatelab/osrm#104, riatelab/osrm#105, riatelab/osrm#106, riatelab/osrm#107, riatelab/osrm#108.

Many of my concerns are a combination of making the package clearer to users, through documentation/examples as well as clarifying the functionality.

elbeejay commented 2 years ago

Fantastic, thanks @wcjochem. @rCarto it seems that all 3 reviews are in, so please have a look and give us a preliminary estimate of when you believe you'll be able to make the necessary changes.

Massive thanks again to @JosiahParry @mikemahoney218 and @wcjochem for providing thoughtful and constructive reviews!

elbeejay commented 2 years ago

@rCarto - Following up here to ask 2 questions:

Have you taken a look at the review comments?
When do you anticipate being able to make the necessary changes to the code and the paper? A rough timeline can help both myself and our reviewers be prepared to revisit this.

rCarto commented 2 years ago

Hello @elbeejay ,

Yes! I plan to implement all these suggestions. Thank you very much to you and the reviewers. These remarks and suggestions will definitely make the package better and ease its usage
I'll do my best and try to address these modifications in the following 2 weeks.

rCarto commented 2 years ago

Hello @elbeejay , @wcjochem , @JosiahParry and @mikemahoney218

Here is the list of modifications I have applied to the package in order to take into account your comments:

State of the field riatelab/osrm#92 I've added a reference about osrmr pkg.
How to use the package riatelab/osrm#92 I've added a section on how to use the package with a short demonstration of key functionalities in the paper and in the README
Tests riatelab/osrm#93 I've extracted some processing steps in their own functions and I've added tests that check the output of internal functions based on fixed input and output files (see test_internals_input_route.R for example).
Inputs management riatelab/osrm#94 riatelab/osrm#95 riatelab/osrm#98 riatelab/osrm#99 I've modified the code to better test inputs. Now, most of the functions accept data.frame of coordinates, matrix of coordinates, sfc and sf objects. I've change the way to set identifiers, row.names (data.frame, matrix, sf obj) or indexes (sfc obj, unnamed matrices) are used.
WGS84 requirement riatelab/osrm#96 riatelab/osrm#97 The documentation was unclear about which input need to use lon/lat wgs84 coordinates. I've tried to modify the documentation to make it more clear. If the input is a non-spatial object (vector of coordinates, data.frames and matrices) it must use WGS84 coordinates and the output use WGS84. If the input is a spatial object (sf or sfc object) it can use any CRS and the output use the input CRS.
osrmTrip() documentation riatelab/osrm#100 The documentation of osrmTrip() has been updated to better explain its output.
Contributing guidelines riatelab/osrm#103 I've added a CONTRIBUTING.md file describing contributing guidelines. This file is referenced at the bottom of the README file along with some shorter guidelines.
osrmRoute() argument order riatelab/osrm#105 osrmRoute() and osrmTable() arguments are now aligned. I'll keep the loc arg for backward compatibility and ease of use.
Define 'isochrones' riatelab/osrm#106 Documentation has been updated to define isochrones.
Difference between osrmRoute() and osrmTrip() riatelab/osrm#107 I've updated the documentation to better explain both functions.
Typos riatelab/osrm#101 riatelab/osrm#104 riatelab/osrm#92 Typos are corrected

elbeejay commented 2 years ago

@editorialbot check references

elbeejay commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

elbeejay commented 2 years ago

Here just to follow-up on @rCarto's message. @wcjochem , @JosiahParry and @mikemahoney218 when y'all get a chance to take another look at this, please do and let us know if your comments have been addressed, or if you think further changes are necessary before this is acceptable for publication.

Thanks! Jay

elbeejay commented 2 years ago

:wave: @wcjochem , @JosiahParry and @mikemahoney218 just wanted to page you all again to make sure returning to this review isn't a task that slips through the cracks!

mikemahoney218 commented 2 years ago

Hey @elbeejay -- sorry for the delay, I'm currently not paying a ton of attention to GitHub or emails (through the 21st). I'll review the changes in the next week.

JosiahParry commented 2 years ago

Thanks for the reminder! I'll be funemployed for a week next week. I'll get to it then.

On Thu, Oct 6, 2022 at 9:27 PM Michael Mahoney @.***> wrote:

Hey @elbeejay https://github.com/elbeejay -- sorry for the delay, I'm currently not paying a ton of attention to GitHub or emails (through the 21st). I'll review the changes in the next week.

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4574#issuecomment-1270991702, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADHIKLBIBIJFKPJNSOOXEI3WB5365ANCNFSM53ZUSM3Q . You are receiving this because you were mentioned.Message ID: @.***>

wcjochem commented 2 years ago

@elbeejay the edits from @rCarto have addressed my original concerns. In testing this new version, I did notice that a new dependency for RcppSimdJson was added. Unfortunately, that caused me problems installing the package on a Windows machine, but it worked fine under Linux. Thanks very much.

elbeejay commented 2 years ago

Thanks for following up @wcjochem, would you mind checking off the remainder of the boxes in your checklist above if those areas that previously needed some work have now been addressed?

Regarding your concern about the new dependency, I appreciate the note about your difficulties installing it on Windows. @rCarto have you experienced this? The CI pipelines do include a Windows build, so it seems like it can be done. If there are known difficulties @rCarto, I would suggest being more descriptive in the installation instructions to help users get a functional installation on Windows machines.

rCarto commented 2 years ago

Thank you for the feedback @wcjochem. I've just tested the installation on 2 Windows machines without problem. I would gladly add more descriptive installation instructions, but I would firstly need a more specific report of the problems (can you tell me more about these problems @wcjochem ?).

JosiahParry commented 2 years ago

Outstanding documentation improvements:

Error handling improvements (though, I don't think this is necessarily a show stopper):

Once the documentation improvements have been made, I'm happy to tick the last box. I do think that better error handling should be incorporated as well, but I don't necessarily think that takes away from the functionality in a significant way. It just improves the end user experience.

mikemahoney218 commented 2 years ago

@elbeejay I think all of my concerns have been addressed :smile: I've checked off the last boxes on my checklist. Good job @rCarto and congrats!

elbeejay commented 2 years ago

Great, thanks all for the follow up. @JosiahParry thank you for explicitly identifying outstanding issues.

wcjochem commented 2 years ago

Thank you for the feedback @wcjochem. I've just tested the installation on 2 Windows machines without problem. I would gladly add more descriptive installation instructions, but I would firstly need a more specific report of the problems (can you tell me more about these problems @wcjochem ?).

@rCarto, thanks for your reply. I have managed to install the package on Windows now. I suspect it was something else out of date preventing the install. I can't reproduce my errors. I've completed the checklist now.

openjournals / joss-reviews