openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: RidePy: A fast and modular framework for simulating ridepooling systems #6241

Closed editorialbot closed 6 months ago

editorialbot commented 10 months ago

Submitting author: !--author-handle-->@fxjung<!--end-author-handle-- (Felix Jung) Repository: https://github.com/PhysicsOfMobility/ridepy Branch with paper.md (empty if default branch): joss Version: v2.7 Editor: !--editor-->@diehlpk<!--end-editor-- Reviewers: @xoolive, @abhishektiwari, @ixjlyons Archive: 10.5281/zenodo.11084287

Status

status

Status badge code:

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

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

@xoolive & @abhishektiwari & @ixjlyons, 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 @diehlpk 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 @xoolive

📝 Checklist for @ixjlyons

📝 Checklist for @abhishektiwari

editorialbot commented 10 months 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 10 months ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.13 s (953.7 files/s, 129906.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          46           1600           2007           7122
Cython                          12            305            284           1275
C/C++ Header                    13            181            283            832
reStructuredText                20            269            251            506
TeX                              1             34              0            470
YAML                            11             36             12            431
C++                              4             52             11            213
TOML                             1             10              4            101
CMake                            9             13             21             54
Markdown                         1             25              0             47
INI                              2              2              0             11
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                           121           2531           2880          11071
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 10 months ago

Wordcount for paper.md is 1121

editorialbot commented 10 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/978-3-662-45079-6 is OK
- 10.1137/141000671 is OK
- 10.1109/MCSE.2010.118 is OK
- 10.1080/03081060.2023.2194874 is OK
- 10.1109/ITSC.2019.8916955 is OK
- 10.48550/arXiv.2207.14246 is OK
- 10.1007/s11116-018-9923-2 is OK
- 10.1016/j.tra.2018.10.028 is OK
- 10.17487/RFC4180 is OK
- 10.1371/journal.pone.0269682 is OK
- 10.1109/ITSC.2018.8569938 is OK
- 10.1088/1367-2630/ac47c9 is OK
- 10.1007/s41109-020-00290-2 is OK
- 10.1103/PhysRevLett.125.248302 is OK
- 10.1016/j.multra.2023.100080 is OK
- 10.1109/ITSC.2018.8569961 is OK
- 10.1109/TITS.2020.2990202 is OK
- 10.1073/pnas.1403657111 is OK
- 10.1038/srep42868 is OK
- 10.25080/Majora-92bf1922-00a is OK
- 10.1177/0361198121997140 is OK
- 10.1038/s41467-023-37728-x is OK
- 10.1038/s41598-022-14960-x is OK
- 10.1016/j.procs.2021.03.083 is OK
- 10.1016/j.tra.2022.09.001 is OK

MISSING DOIs

- 10.1007/978-1-4842-2677-3_5 may be a valid DOI for title: Pytest

INVALID DOIs

- None
editorialbot commented 10 months ago

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

ixjlyons commented 10 months ago

Review checklist for @ixjlyons

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

xoolive commented 10 months ago

Review checklist for @xoolive

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

xoolive commented 10 months ago

Dear @fxjung,

Before I start trying to go into the code, I am having serious concerns about the quality of the paper and of the documentation. I entered your project from these two elements, and I am just confused. On both sides, I am having a hard time understanding what the library is about: I get the context, but not really what the library would provide me as an end user. It talks about simulation but then... I am a bit lost.

As an example, you start your documentation by saying what ridepy does not do, this is not the most helpful way to catch your potential users.

To be honest, I do not plan to put more efforts in the review until those two key elements are fully clarified.

Also, as a minor comment that would come back later: I do not recommend inflating your bibliography with citations keys for programming languages and widely adopted libraries (such as Pandas). It is commonly accepted that software papers do not have as many references as standard academic/methodological papers.

xoolive commented 10 months ago

Also please consider the item in our reviewer checklist for automated tests.

fxjung commented 10 months ago

Dear @fxjung,

Before I start trying to go into the code, I am having serious concerns about the quality of the paper and of the documentation. I entered your project from these two elements, and I am just confused. On both sides, I am having a hard time understanding what the library is about: I get the context, but not really what the library would provide me as an end user. It talks about simulation but then... I am a bit lost.

As an example, you start your documentation by saying what ridepy does not do, this is not the most helpful way to catch your potential users.

To be honest, I do not plan to put more efforts in the review until those two key elements are fully clarified.

Also, as a minor comment that would come back later: I do not recommend inflating your bibliography with citations keys for programming languages and widely adopted libraries (such as Pandas). It is commonly accepted that software papers do not have as many references as standard academic/methodological papers.

Dear @xoolive,

Thank you for reviewing our paper and for your comments.

I am sorry for any confusion regarding the purpose of the library. I have now tried to make the description of the functionality more accessible/high-level in the overview section. Furthermore, the documentation now additionally mentions the introductory jupyter notebooks under Quickstart. Finally, I have updated the README, moving possibly confusing internals at the end to a separate glossary chapter in the documentation and highlighting the broad structure of the documentation at the beginning. I hope this addresses your concerns.

Unfortunately, I think I do not fully understand your criticism regarding the manuscript. We have tried to point out as clear as possible what the library does and what it's useful for both in the Statement of need and Philosophy of usage sections. It would be great if you could elaborate a bit further on what exactly may be missing here.

Coming from physics, I assumed it would be sensible to cite even popular languages and libraries. As you implied that this is uncommon in software literature, I have removed them accordingly.

Regarding your comment on automated tests: The project is already extensively tested using pytest. The tests are located in the test folder as configured in pytest.ini. These tests are automatically run using GitHub actions and may also be run locally by executing pytest in the repository root. Manually testing whether the software works is also possibly using the example notebooks in the notebooks directory.

Felix

xoolive commented 10 months ago

Thank you @fxjung

I am sorry I spoke too fast about the tests, I just missed them :fearful: I will have a look!

To be honest, I still recommend improving the website for the documentation. Avoid twisting the user into opening jupyter notebooks (I never do), but you can render them with sphynx, there's a plugin for that. Maybe it can help to demonstrate the capabilities of the library.

I am not expecting a perfect documentation, it just takes too much time to write an excellent one, but the fact is that I am missing the point there. This is my favourite resource for documentation by the way.

fxjung commented 9 months ago

@xoolive Thanks a lot for pointing out the documentation system guide! This is a very helpful reference, indeed. I've tried to restructure and improve the documentation over the course of the last days, the current version is up on ridepy.org. It doesn't fully adapt the scheme (yet), technical reference and explanations are still a bit intertwined, and there is only one technical how-to guide.

I have integrated the jupyter notebooks in the documentation, that's actually a great suggestion. Also, I have refactored/rewritten substantial parts of them.

I hope, that the library and its documentation are much more accessible and understandable now.

xoolive commented 9 months ago

@fxjung the documentation looks quite good! I think I am ok with what's left. Consider regenerating the pdf to help fellow reviewers checking the latest version of the statement paper.

fxjung commented 9 months ago

@xoolive Very happy to hear that!

fxjung commented 9 months ago

@editorialbot generate pdf

editorialbot commented 9 months ago

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

abhishektiwari commented 9 months ago

Review checklist for @abhishektiwari

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

diehlpk commented 9 months ago

Hi @ixjlyons how is your review going?

diehlpk commented 8 months ago

Hi @ixjlyons how is your review going?

abhishektiwari commented 8 months ago

@fxjung Great work. Overall I enjoyed reviewing this software. Please see my initial comments (all of them non-blocking),

  1. Please consider improving the Github repo Readme by adding installation instructions and contribution guidelines. I know these are already covered by documentation portal in great details but given Github repo is landing page for this software paper I highly recommend to update Readme.
  2. Please consider improving the A statement of need and Summary covered by Github repo Readme and documentation portal landing page, and align them with paper. I feel they all read differently and only after reading the software paper or the conceptual overview one can understand the problem statement and target audience.
  3. As mentioned by paper ridepy's modular design makes customization easy, while the included modules allow for a quick start, documentation will benefit with examples of how to extend the ridepy modules with a real-world use case. API/code documentation for supported dispatching algorithms partially covers this but additional details will help users.
  4. Installation instructions worked but can be little bit clear by calling out pre-installation of C++ build environment and the Boost libraries. For instance, on Debian run sudo apt update && sudo apt -y install build-essential libboost-all-dev before pip install ridepy.

I commend authors effort to document a domain glossary to help anyone not familiar with the space to rationalise it quickly. There is already a comprehensive backlog of improvements documented in issue tracker so it seems there is additional unit tests planned to improve the test coverage.

@diehlpk Will wait for response from @fxjung, but review is mostly completed from my end. Please let me know if anything else required.

fxjung commented 8 months ago

@abhishektiwari Thanks a lot for the positive review and the helpful comments! I will definitely try to implement them.

fxjung commented 8 months ago

@abhishektiwari Regarding your second comment, I am not fully sure whether I understand it correctly. Could you go into a bit more detail what exactly you feel is missing from which part of the documentation?

I don't really want to repeat the more in-depth explanations of the conceptual overview on the landing page or in the README. Do you suggest inserting just a short summary of the conceptual overview? Or do you rather think that points we made in the manuscript are missing from the conceptual overview/landing page/README?

ixjlyons commented 8 months ago

Hi @ixjlyons how is your review going?

My apologies to everyone here - it was going great until I put it down to pick up later. I will get the review done this week.

diehlpk commented 8 months ago

@fxjung have you addressed @abhishektiwari's comments?

fxjung commented 8 months ago

@diehlpk Sorry I didn't mention this in my reply above---yes, I have tried to address them: I have added quick-start and contribution sections to the README, a guide in the docs on how to write a custom dispatcher, and I have improved the installation instructions.

Regarding the second comment I was not really sure what to do as I pointed out in my earlier reply.

diehlpk commented 8 months ago

@abhishektiwari can you please elaborate on the second d comment?

abhishektiwari commented 8 months ago

@diehlpk Let me respond by tomorrow. In my TODO list.

ixjlyons commented 8 months ago

My review:

Overall

Overall, the code, documentation, and paper are put together well. A lot of effort has gone into performance, and it seems to be worthwhile from the (admittedly cursory) testing I did. I only have some minor comments.

Paper

The opening summary section/paragraph could probably benefit from some description of analyses enabled by the software, mainly for non-specialist readers. This is my only hangup on the checklist, though arguably the "Philosopy and usage" section covers it.

Also in the summary section, there's a distinction made between "modeling the mobility service itself rather than its customers or the environment." I don't think a full justification fits here, but it could be helpful to point out what benefits this design decision has, whether it improves modeling flexibility, "naturalness" of model specification, performance, etc. I could imagine the sentence continuing like "...rather than its customers or environment, enabling ..."

~The citation of the Julia language seems superfluous to me, since it's only mentioned in reference to another package.~ The package is also referred to as RidePooling.jl, but I'm not seeing any package by that name. I think the description "...developed in support of a recent scientific contribution" is vague enough to not really be useful. If the idea is to point out that it's academic (what I'm guessing from "idealized approach"), it might be good to specify some feature(s) RidePy offers, or something to that effect.

Documentation

I had a few comments on the documentation, but I see there's been a bit of an overhaul and I think my comments were addressed.

I also just want to point out the new page on writing a custom dispatcher model is really nice - more libraries need this kind of thing.

abhishektiwari commented 8 months ago

@fxjung Thanks for revisions and update.

Re: Comment-2 Please consider comment it resolved from my side. I see you already have revised the statement of need/summary on Readme and documentation portal. Re: comment-3 Details documentation on how to write a custom dispatcher looks great. Thanks for putting extra effort there. Users will love it. Re: Comment-1 and Comment-4 are also addressed.

@diehlpk No open issues or comments from my side.

diehlpk commented 8 months ago

Paper

The opening summary section/paragraph could probably benefit from some description of analyses enabled by the software, mainly for non-specialist readers. This is my only hangup on the checklist, though arguably the "Philosopy and usage" section covers it.

Also in the summary section, there's a distinction made between "modeling the mobility service itself rather than its customers or the environment." I don't think a full justification fits here, but it could be helpful to point out what benefits this design decision has, whether it improves modeling flexibility, "naturalness" of model specification, performance, etc. I could imagine the sentence continuing like "...rather than its customers or environment, enabling ..."

~The citation of the Julia language seems superfluous to me, since it's only mentioned in reference to another package.~ The package is also referred to as RidePooling.jl, but I'm not seeing any package by that name. I think the description "...developed in support of a recent scientific contribution" is vague enough to not really be useful. If the idea is to point out that it's academic (what I'm guessing from "idealized approach"), it might be good to specify some feature(s) RidePy offers, or something to that effect.

@fxjung can you please have a look at this comment?

fxjung commented 8 months ago

@abhishektiwari Very happy to hear that! Also, I'm very glad that you and @ixjlyons are happy with the new dispatcher guide, more guides will definitely follow in the future.

@ixjlyons Thanks for the great review, and for the helpful comments. @diehlpk I will try to improve the paper accordingly and let you know when I'm done.

diehlpk commented 8 months ago

@ixjlyons there is one tick missing on your check list. Can you please have a look?

diehlpk commented 7 months ago

@ixjlyons there is one tick missing on your check list. Can you please have a look?

ixjlyons commented 7 months ago

I left it unchecked pending an update to the summary description in the paper.

diehlpk commented 7 months ago

@fxjung can you please have a look at the comment above?

fxjung commented 7 months ago

@diehlpk Still working on it, will post an update as soon as we're done.

diehlpk commented 7 months ago

@fxjung can you estimate when you are done with the changes?

fxjung commented 7 months ago

@editorialbot generate pdf

editorialbot commented 7 months ago

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

fxjung commented 7 months ago

@diehlpk @ixjlyons Thanks again for your helpful suggestions, and sorry for the delay! My coauthor and I have now tried to update the manuscript accordingly.

diehlpk commented 7 months ago

@ixjlyons can you please have a look at the changes?

fxjung commented 7 months ago

@editorialbot generate pdf

editorialbot commented 7 months ago

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

ixjlyons commented 7 months ago

Changes to the paper look good to me :+1:

diehlpk commented 7 months ago

@xoolive, @abhishektiwari, @ixjlyons thanks for finishing your reviews.

diehlpk commented 7 months ago

@editorialbot commands

editorialbot commented 7 months ago

Hello @diehlpk, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Add to this issue's reviewers list
@editorialbot add @username as reviewer

# Remove from this issue's reviewers list
@editorialbot remove @username from reviewers

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Assign a user as the editor of this submission
@editorialbot assign @username as editor

# Remove the editor assigned to this submission
@editorialbot remove editor

# Remind an author, a reviewer or the editor to return to a review after a 
# certain period of time (supported units days and weeks)
@editorialbot remind @reviewer in 2 weeks

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for version
@editorialbot set v1.0.0 as version

# Set a value for branch
@editorialbot set joss-paper as branch

# Set a value for repository
@editorialbot set https://github.com/organization/repo as repository

# Set a value for the archive DOI
@editorialbot set 10.5281/zenodo.6861996 as archive

# Mention the EiCs for the correct track
@editorialbot ping track-eic

# Run checks and provide information on the repository and the paper file
@editorialbot check repository

# Check the references of the paper for missing DOIs
@editorialbot check references

# Generates the pdf paper
@editorialbot generate pdf

# Recommends the submission for acceptance
@editorialbot recommend-accept

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Flag submission with questionable scope
@editorialbot query scope

# Get a link to the complete list of reviewers
@editorialbot list reviewers

# Creates a post-review checklist with editor and authors tasks
@editorialbot create post-review checklist

# Open the review issue
@editorialbot start review
diehlpk commented 7 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

diehlpk commented 7 months ago

@editorialbot check references

editorialbot commented 7 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/978-3-662-45079-6 is OK
- 10.1137/141000671 is OK
- 10.1109/MCSE.2010.118 is OK
- 10.1080/03081060.2023.2194874 is OK
- 10.1109/ITSC.2019.8916955 is OK
- 10.48550/arXiv.2207.14246 is OK
- 10.1007/s11116-018-9923-2 is OK
- 10.1016/j.tra.2018.10.028 is OK
- 10.17487/RFC4180 is OK
- 10.1371/journal.pone.0269682 is OK
- 10.1109/ITSC.2018.8569938 is OK
- 10.1088/1367-2630/ac47c9 is OK
- 10.1007/s41109-020-00290-2 is OK
- 10.1103/PhysRevLett.125.248302 is OK
- 10.1109/ITSC.2018.8569961 is OK
- 10.1109/TITS.2020.2990202 is OK
- 10.1073/pnas.1403657111 is OK
- 10.1038/srep42868 is OK
- 10.25080/Majora-92bf1922-00a is OK
- 10.1177/0361198121997140 is OK
- 10.1038/s41467-023-37728-x is OK
- 10.1038/s41598-022-14960-x is OK
- 10.1016/j.procs.2021.03.083 is OK
- 10.1016/j.tra.2022.09.001 is OK

MISSING DOIs

- No DOI given, and none found for title: The Multi-Agent Transport Simulation MATSim
- No DOI given, and none found for title: Lyft
- 10.1007/978-1-4842-2677-3_5 may be a valid DOI for title: Pytest
- No DOI given, and none found for title: RidePy Documentation
- No DOI given, and none found for title: PhysicsOfMobility/Ridepy - Github
- No DOI given, and none found for title: Ridepy - PyPI
- No DOI given, and none found for title: The C++ Programming Language
- No DOI given, and none found for title: Uber
- No DOI given, and none found for title: Python Reference Manual

INVALID DOIs

- None