joss-reviews icon indicating copy to clipboard operation
joss-reviews copied to clipboard

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

Open editorialbot opened this issue 5 months ago • 30 comments

Submitting author: @fxjung (Felix Jung) Repository: https://github.com/PhysicsOfMobility/ridepy Branch with paper.md (empty if default branch): joss Version: v2.1.post4 Editor: @diehlpk Reviewers: @xoolive, @abhishektiwari, @ixjlyons Archive: Pending

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 avatar Jan 18 '24 19:01 editorialbot

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 avatar Jan 18 '24 19:01 editorialbot

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 avatar Jan 18 '24 19:01 editorialbot

Wordcount for paper.md is 1121

editorialbot avatar Jan 18 '24 19:01 editorialbot

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 avatar Jan 18 '24 19:01 editorialbot

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

editorialbot avatar Jan 18 '24 19:01 editorialbot

Review checklist for @ixjlyons

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

General checks

  • [x] Repository: Is the source code for this software available at the https://github.com/PhysicsOfMobility/ridepy?
  • [x] License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?
  • [x] Contribution and authorship: Has the submitting author (@fxjung) 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
  • [x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • [x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • [x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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?

ixjlyons avatar Jan 18 '24 20:01 ixjlyons

Review checklist for @xoolive

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

General checks

  • [x] Repository: Is the source code for this software available at the https://github.com/PhysicsOfMobility/ridepy?
  • [x] License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?
  • [x] Contribution and authorship: Has the submitting author (@fxjung) 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
  • [x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • [x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • [x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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?

xoolive avatar Jan 19 '24 14:01 xoolive

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 avatar Jan 23 '24 22:01 xoolive

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

xoolive avatar Jan 23 '24 22:01 xoolive

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

fxjung avatar Jan 25 '24 17:01 fxjung

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.

xoolive avatar Jan 26 '24 10:01 xoolive

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

fxjung avatar Feb 07 '24 16:02 fxjung

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

xoolive avatar Feb 13 '24 21:02 xoolive

@xoolive Very happy to hear that!

fxjung avatar Feb 15 '24 13:02 fxjung

@editorialbot generate pdf

fxjung avatar Feb 15 '24 13:02 fxjung

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

editorialbot avatar Feb 15 '24 13:02 editorialbot

Review checklist for @abhishektiwari

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

General checks

  • [x] Repository: Is the source code for this software available at the https://github.com/PhysicsOfMobility/ridepy?
  • [x] License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?
  • [x] Contribution and authorship: Has the submitting author (@fxjung) 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
  • [x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • [x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • [x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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?

abhishektiwari avatar Feb 19 '24 00:02 abhishektiwari

Hi @ixjlyons how is your review going?

diehlpk avatar Feb 29 '24 01:02 diehlpk

Hi @ixjlyons how is your review going?

diehlpk avatar Mar 07 '24 15:03 diehlpk

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

abhishektiwari avatar Mar 09 '24 18:03 abhishektiwari

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

fxjung avatar Mar 11 '24 15:03 fxjung

@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?

fxjung avatar Mar 11 '24 16:03 fxjung

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.

ixjlyons avatar Mar 18 '24 17:03 ixjlyons

@fxjung have you addressed @abhishektiwari's comments?

diehlpk avatar Mar 22 '24 15:03 diehlpk

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

fxjung avatar Mar 22 '24 16:03 fxjung

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

diehlpk avatar Mar 25 '24 17:03 diehlpk

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

abhishektiwari avatar Mar 25 '24 18:03 abhishektiwari

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.

ixjlyons avatar Mar 26 '24 05:03 ixjlyons

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

abhishektiwari avatar Mar 27 '24 01:03 abhishektiwari

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?

diehlpk avatar Mar 27 '24 20:03 diehlpk