search

openjournals / joss-reviews

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

[REVIEW]: DRSP-Sim: A Simulator for Ride-Sharing with Pooling: Joint Matching, Pricing, Route Planning, and Dispatching #3761

Closed whedon closed 1 year ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@marina-haliem<!--end-author-handle-- (Marina Haliem) Repository: https://github.com/marina-haliem/Dynamic-RideSharing-Pooling-Simulator Branch with paper.md (empty if default branch): Version: v1.1 Editor: !--editor-->@martinfleis<!--end-editor-- Reviewers: @RafalKucharskiPK, @martinfleis, @jGaboardi Archive: Pending

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@RafalKucharskiPK & @martinfleis, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @martinfleis 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 ✨

Review checklist for @RafalKucharskiPK

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @martinfleis

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @RafalKucharskiPK, @1tylermitchell it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands

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

@whedon generate pdf
whedon commented 3 years ago

Wordcount for paper.md is 1528

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/INFOCOM.2018.8485988 is OK
- 10.1109/TITS.2019.2931830 is OK
- 10.1073/pnas.1611675114 is OK
- 10.1016/j.cor.2016.07.020 is OK
- 10.1007/s12469-016-0139-6 is OK
- 10.1016/j.ejor.2012.05.028 is OK
- 10.1145/3385958.3430484 is OK
- 10.1109/TITS.2021.3096537 is OK
- 10.1109/TITS.2020.3048361 is OK
- 10.1109/tits.2021.3083740 is OK
- 10.1145/3408308.3431114 is OK
- 10.1007/s11116-019-10012-y is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.trc.2020.02.008 is INVALID because of 'https://doi.org/' prefix
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.10 s (470.3 files/s, 54457.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          42            753            958           3366
Markdown                         2             71              0            198
TeX                              1             16              0            194
YAML                             1             14              4             74
XML                              3              0              0             26
-------------------------------------------------------------------------------
SUM:                            49            854            962           3858
-------------------------------------------------------------------------------

Statistical information for the repository '191604a27c7b329dadbed4f1' was
gathered on 2021/09/23.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Marina Wagdy Wadea H            17         15361           1240          100.00

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Haliem, Marina Wagdy          4          100.0          4.8                0.00
Marina Wagdy Wadea H       5073           33.0          0.7               17.94
whedon commented 3 years ago

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

martinfleis commented 3 years ago

👋🏼 @marina-haliem @RafalKucharskiPK @1tylermitchell this is the review thread for the paper. All of our communications will happen here from now on.

Both reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#3761 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@martinfleis) if you have any questions/concerns.

martinfleis commented 3 years ago

I am copying here https://github.com/openjournals/joss-reviews/issues/3712#issuecomment-924912823 from @RafalKucharskiPK for completeness. 👇

Dear @marina-haliem I cloned the repo, downloaded the data (from https://purr.purdue.edu/publications/3843/1), created conda env, installed most of dependencies and run simulator_driver.py.

Minor issues that I managed to overcome:

  1. you require pandas==0.22 - which is ancient - 2017(!), and it clashed with many of my other packages, I used lates pandas and so far it went OK.

  2. The db.sqlite3 path is your local path, I changed it to "data\db": image

  3. The default simulation takes 8 days (in simulation) one day takes ca. one hour on my laptop - way too long for a quickstart, please provide a toy example for first users.

Nonetheless it did not work for me, after a single day the error occured. Then I changed to one day in config, and again after a day passed I receive:

Traceback (most recent call last):
  File "/Users/rkucharski/Documents/GitHub/Dynamic-RideSharing-Pooling-Simulator/simulator_driver.py", line 159, in <module>
    Sim_experiment.simulator.step()
  File "/Users/rkucharski/Documents/GitHub/Dynamic-RideSharing-Pooling-Simulator/simulator/simulator.py", line 71, in step
    if vehicle.agent_type == agent_codes.dqn_agent:
AttributeError: 'Vehicle' object has no attribute 'agent_type'

Process finished with exit code 1

so please create a stable version to reproduce.

PS. On the main github page I am missing quick walkthrough tutorial for first users, e.g. now I first see how to set 'OSRM' which happens to be not needed, but only later I see where is the data bundle to run simulations.

Good luck!

marina-haliem commented 3 years ago

Hi @RafalKucharskiPK,

Thank you for your comments. I have updated the requirements list, and the default db_dir path. Also, I've updated the Getting Started section, highlighting how to run a quick start of 1 day. Apologies regarding this error, there was a minor bug that I fixed. I have conducted some additional testing, and I believe it should run smoothly now for you.

Please, let us know if you come across any further issues.

Thank you, Marina

whedon commented 3 years ago

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

whedon commented 3 years ago

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

RafalKucharskiPK commented 3 years ago

@marina-haliem Thanks, I managed to run the simulation, create logs and parse them. Now I will take a look at the functionalities.

Issues I had:

I will get back when I analyze and understand the results.

marina-haliem commented 3 years ago

Thanks, @RafalKucharskiPK, for your comments.

Thank you, Marina

martinfleis commented 3 years ago

Hi @1tylermitchell @RafalKucharskiPK,

can you give me an indication of how much time do you need to finish the review (following the checklist above)? It's currently 8 weeks since the review started and it would be great if we could wrap this soon so @marina-haliem can work on the revisions.

Thanks!

martinfleis commented 2 years ago

@whedon remove @1tylermitchell as reviewer

whedon commented 2 years ago

OK, @1tylermitchell is no longer a reviewer

RafalKucharskiPK commented 2 years ago

Hi, sorry about that - I hope to give you feedback before christmas. I already run simulations and took a look at the text. PS. Do we wait for another reviewer or does it rely on my opinion only now?

martinfleis commented 2 years ago

Thanks, @RafalKucharskiPK!

Do we wait for another reviewer or does it rely on my opinion only now?

I am looking for another reviewer at the moment.

martinfleis commented 2 years ago

@whedon add @martinfleis as reviewer

whedon commented 2 years ago

OK, @martinfleis is now a reviewer

martinfleis commented 2 years ago

@marina-haliem I have formally added myself as a second reviewer now (it is not a common solution but possible). I hope that it will help speeding up the review a bit as I don't expect that any new reviewer would be able to work on it before the upcoming holidays.

RafalKucharskiPK commented 2 years ago

in this case I also may commit to provide a review before end of the year

martinfleis commented 2 years ago

@RafalKucharskiPK That would be amazing! Thanks!

martinfleis commented 2 years ago

Hi @marina-haliem ,

First, apologies for the long review process. It is not always possible to make it swift, especially with more common unexpected issues these days.

I have started my review of DRSP-Sim and ticked relevant boxes in the checklist above. However, I cannot at the moment tick the rest, in some cases due to missing components of the submission, in others due to (I assume) technical limitations on my side.

Installation

Example usage and Functionality documentation

Automated tests

Community guidelines

Apart from these points from the checklist, I also have a couple of additional questions and a few minor comments.

Some of these points (documentation, tests) will undoubtedly require some work but you will need to address them to meet the JOSS review criteria.

Let me know if there’s anything unclear in my comments.

RafalKucharskiPK commented 2 years ago

Dear Editor, Dear Authors @marina-haliem ,

sorry for the delayed process and sorry that you did not manage to collect more reviewers. I believe we may divide the review process into two separate parts:

  1. Paper
  2. Software

Since the software is well reviewed by @martinfleis and I had similar issues at the installation, I will focus more on the paper:

  1. I am missing the use-cases. You well review the existing alternative models and highlight novelty, but I am still missing use-cases
  2. The functionality of modules used in the architecture is missing. Why do we have them - how can we use them (parameterise) - and what are the use-cases for applying them in studies.
  3. All in all the paper is very brief and does not give an overview of what is the software - it focuses a bit why is it better than other similar ones, but misses the point on what it really is.
  4. The structure is misleading for me: you have Benchmarks as the section and then under Benchmarks you have details of pricing and mathing algorithms in subsections - are they benchmarks?

Please try to improve the clarity and completeness of the paper. By the way the issues are very similar to the issues found in the software description.

Software: To summarize my previous issues - I am missing a walkthrough tutorial: starting from uses-case scenario (what do we want to simulate), through set-up, input data - through running and up to interpreation of the results. Without this I'd miss the whole story.

Hope this helps improve the paper! Rafal Kucharski

martinfleis commented 2 years ago

Hi @marina-haliem,

Now that you have both reviews of the first round, please let us know how you plan to respond to the comments raised by reviewers and what is the expected timeline for that. Thanks!

Thanks @RafalKucharskiPK for your comments!

martinfleis commented 2 years ago

@RafalKucharskiPK Can you please update your checklist above?

RafalKucharskiPK commented 2 years ago

@RafalKucharskiPK Can you please update your checklist above?

what do you mean exactly @martinfleis ?

martinfleis commented 2 years ago

@RafalKucharskiPK In the very top post in this issue is an automatically crated review checklist prepared for you. We use it to ensure that the paper fulfils all the JOSS requirements and wait until all boxes are ticked before accepting it. You should be able to edit that post and tick boxes you think the current version of the software and the paper meets. If you are unable to do so, I might need to re-invite you to join the JOSS org.

marina-haliem commented 2 years ago

Thank you for your comments and feedback.

Reviewer 1 @martinfleis :

Installation Would you elaborate on what issues are facing with this? I have personally run this project on a Mac laptop without any problems.

Example usage and Functionality documentation

  1. I have documented the functionalities within the source code for more clarity as well as adding a thorough explanation of each module's functionalities in the paper under the (Simulator Architecture and Functionalities Section).
  2. The modular code architecture diagram has also been updated to include all of the core functionalities of each sub-module.

Testing For testing, we have provided manual steps in the Readme document that can be followed along with a sample expected output to check the expected functionality of the software.

Community guidelines The developer guidelines are provided in the Readme including how to set up a dev environment, run the simulator, and generate plots to interpret the logs. No special code style is needed, just regular python code.

  1. This codebase was initially developed over the old Purdue Github link, co-authors commits would have shown there. This Github link was mainly created for this submission.
  2. Repository link updated in the Readme. We intend to move the repository to another Github link in the future. Formatting of the code snippets in the Readme is broken in some cases (code is formatted as a standard text).
  3. This can be found under the (Data Generation) section in the Readme. If you would like to simulate any other city map (other than NY), you basically follow the same preprocessing steps listed there, but using the osm data of the required city as well as its corresponding trip data as well to generate the discretized city map.
  4. I've updated parse_results.py and log_analyzer.py to avoid confusion of the parameters as @RafalKucharskiPK pointed out above. By default, the parse_results.py will provide a tool to plot the results of the default log directory specified in 'DEFAULT_LOG_DIR'. Since the user can decide to compare any number of experiments without any constraints, it is left up to them to specify their relevant paths to generate relevant plots. I have also updated the relevant description in the README file for more clarity. I am not sure if Arm-based machines processors would cause any changes, but my team and I have successfully ran this project several times.

Thank you, Marina

marina-haliem commented 2 years ago

Thank you for your comments and feedback.

Reviewer 2 @RafalKucharskiPK :

Software: In the Readme, under the (Getting Stated) section, we provide a step-by-step explanation of creating a dev environment, setting up the simulator parameters that provide wide flexibility, loading and defining the input data, running the simulator, storing logs, post-processing and generating plots to interpret these logs. We also provide sample output of what plots would look like, if it were to compare two algorithms and if it were to visualize a summary of metrics for one algorithm.

Thank you, Marina

martinfleis commented 2 years ago

Hi @marina-haliem,

thank you for all the changes and a thorough response!

Would you elaborate on what issues are facing with this? I have personally run this project on a Mac laptop without any problems.

Yes. When following the installation instructions, after cloning the repository, running

pip install -r requirements.txt

in a fresh environment fails with the following error message.

Collecting psycopg2-binary==2.7.4
  Using cached psycopg2-binary-2.7.4.tar.gz (426 kB)
  Preparing metadata (setup.py) ... error
  ERROR: Command errored out with exit status 1:
   command: /Users/martin/mambaforge/bin/python3.9 -c 'import io, os, sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-install-yvq80uz5/psycopg2-binary_d9f6f120662c49d7899e55e339bbb9ea/setup.py'"'"'; __file__='"'"'/private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-install-yvq80uz5/psycopg2-binary_d9f6f120662c49d7899e55e339bbb9ea/setup.py'"'"';f = getattr(tokenize, '"'"'open'"'"', open)(__file__) if os.path.exists(__file__) else io.StringIO('"'"'from setuptools import setup; setup()'"'"');code = f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base /private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-pip-egg-info-ku_fu6e_
       cwd: /private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-install-yvq80uz5/psycopg2-binary_d9f6f120662c49d7899e55e339bbb9ea/
  Complete output (23 lines):
  running egg_info
  creating /private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-pip-egg-info-ku_fu6e_/psycopg2_binary.egg-info
  writing /private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-pip-egg-info-ku_fu6e_/psycopg2_binary.egg-info/PKG-INFO
  writing dependency_links to /private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-pip-egg-info-ku_fu6e_/psycopg2_binary.egg-info/dependency_links.txt
  writing top-level names to /private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-pip-egg-info-ku_fu6e_/psycopg2_binary.egg-info/top_level.txt
  writing manifest file '/private/var/folders/2f/fhks6w_d0k556plcv3rfmshw0000gn/T/pip-pip-egg-info-ku_fu6e_/psycopg2_binary.egg-info/SOURCES.txt'

  Error: pg_config executable not found.

  pg_config is required to build psycopg2 from source.  Please add the directory
  containing pg_config to the $PATH or specify the full executable path with the
  option:

      python setup.py build_ext --pg-config /path/to/pg_config build ...

  or with the pg_config option in 'setup.cfg'.

  If you prefer to avoid building psycopg2 from source, please install the PyPI
  'psycopg2-binary' package instead.

  For further information please check the 'doc/src/install.rst' file (also at
  <http://initd.org/psycopg/docs/install.html>).

  ----------------------------------------
WARNING: Discarding https://files.pythonhosted.org/packages/77/09/4991fcd9a8f4bea1ee3948e1729fa17c184d25bd10809bacc143626361b9/psycopg2-binary-2.7.4.tar.gz#sha256=de4f88f823037a71ea5ef3c1041d96b8a68d73343133edda684fd42f575bd9d7 (from https://pypi.org/simple/psycopg2-binary/). Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.
ERROR: Could not find a version that satisfies the requirement psycopg2-binary==2.7.4 (from versions: 2.7.4, 2.7.5, 2.7.6, 2.7.6.1, 2.7.7, 2.8, 2.8.1, 2.8.2, 2.8.3, 2.8.4, 2.8.5, 2.8.6, 2.9, 2.9.1, 2.9.2, 2.9.3)
ERROR: No matching distribution found for psycopg2-binary==2.7.4

The version 2.7.4 you, for some reason, require is quite old (Feb 2018) and I believe that is causing the issue when you try to install it on Macs with M1 chips. There were no dependencies built for these chips back then.

marina-haliem commented 2 years ago

Hi @martinfleis , Thank you for the clarification. I have updated the requirements.txt. Hopefully, this fixes the problem, I have verified it is working as expected on different machines that I have access to.

Thank you, Marina

marina-haliem commented 2 years ago

Hi @martinfleis , Any updates regarding this submission?

Thank you, Marina

martinfleis commented 2 years ago

@marina-haliem It is waiting for the second round of revisions, sorry for delays.

@RafalKucharskiPK can you have a look at the response above and update the checklist in the first post?

arfon commented 2 years ago

:wave: @martinfleis – looks like this review has stalled somewhat?

martinfleis commented 2 years ago

Hey @RafalKucharskiPK, will you be able to check the response to your comments and update the checklist?

martinfleis commented 2 years ago

Hey @JGaboardi, sorry ti pick you up again :). It seems that we have lost a reviewer along the way here. Would you be willing to help out and review this submission? You will see that there already was one round of reviews done.

jGaboardi commented 2 years ago

@whedon generate pdf

editorialbot commented 2 years ago

My name is now @editorialbot

jGaboardi 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:

martinfleis commented 2 years ago

@editorialbot add @jGaboardi as reviewer

editorialbot commented 2 years ago

@jGaboardi added to the reviewers list!

jGaboardi commented 2 years ago

@martinfleis, will the editorialbot generate a fresh checklist for me or shall I use the one from the previous reviewer?

martinfleis commented 2 years ago

Use @editorialbot generate my checklist command. I didn't realise this review is pre-editorialbot.

jGaboardi commented 2 years ago

Review checklist for @jGaboardi

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

jGaboardi commented 2 years ago

@marina-haliem @martinfleis

Before continuing with my review, I would like to raise two fairly large points (IMHO), that need to be addressed:

martinfleis commented 2 years ago

@marina-haliem Can you respond to comments raised by @jGaboardi above? Please keep in mind that the review process of JOSS is different than in a standard journal and assumes direct communication rather than fixed rounds of review-response cycle. Thank you!

martinfleis commented 1 year ago

@marina-haliem Hi, I understand this review may not go as expected and I apologise for that. However, we need your cooperation at this stage to move forward, so I am just sending a gentle ping to respond to the comments above. Thank you!

oliviaguest commented 1 year ago

@martinfleis it might be worth emailing @marina-haliem?