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]: DE-Sim: an object-oriented, discrete-event simulation tool for data-intensive modeling of complex systems in Python #2685

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @artgoldberg (Arthur Goldberg) Repository: https://github.com/KarrLab/de_sim Version: 1.0.5 Editor: @danielskatz Reviewers: @gonsie, @carothersc, @yadudoc Archive: 10.5281/zenodo.4274852

: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/e3ca43be9717d153672c48239939e993"><img src="https://joss.theoj.org/papers/e3ca43be9717d153672c48239939e993/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/e3ca43be9717d153672c48239939e993/status.svg)](https://joss.theoj.org/papers/e3ca43be9717d153672c48239939e993)

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

@gonsie & @carothersc, 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 @danielskatz 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 @gonsie

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @carothersc

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @yadudoc

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @gonsie, @carothersc 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 4 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1145/2901378.2901402 is OK
- 10.1177/003754978704900506 is OK
- 10.1109/DATE.2001.915002 is OK
- 10.1109/WSC.2003.1261534 is OK
- 10.1109/PADS.2000.847144 is OK
- 10.1016/j.copbio.2017.12.013 is OK
- 10.25080/majora-92bf1922-00a is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1016/j.mib.2015.06.004 is OK
- 10.1016/j.cell.2012.05.044 is OK
- 10.1109/wodes.2006.382398 is OK
- 10.1109/wsc.2005.1574302 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

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

danielskatz commented 4 years ago

👋 @gonsie, @carothersc - Thanks again for agreeing to review. Please read the comments above before starting.

This is the review thread for the paper. All of our communications will happen here from now on.

All 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#2685 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 (@danielskatz) if you have any questions/concerns.

danielskatz commented 4 years ago

Thanks for getting started @gonsie @carothersc, let me know if you need anything on your side

artgoldberg commented 4 years ago

Hi @danielskatz. @gonsie finds that one of DE Sim's unit tests fails because of a problem in the capturer testing library. The test passes on multiple platforms (local, Docker on local, Docker in CircleCI) for me and I have not reproduced the capturer failure thus far. See KarrLab/de_sim#55 for the details.

How would you suggest we proceed?

Thanks Arthur

danielskatz commented 4 years ago

It seems to me that either there is a problem that should be discussed in the documentation, with a workaround, or something else.

artgoldberg commented 4 years ago

Thanks @danielskatz

OK. I have replaced the failing code in DE Sim's tests with a method from Python's standard library that will hopefully avoid the problem. See KarrLab/de_sim#55 for further details.

Arthur

artgoldberg commented 4 years ago

Hello @danielskatz

I'm available to address any other issues reviewers raise.

Arthur

danielskatz commented 4 years ago

@gonsie & @carothersc - can I get an update on your review status? It would be great to get this moving a bit further along...

danielskatz commented 4 years ago

@whedon re-invite @carothersc as reviewer

whedon commented 4 years ago

The reviewer already has a pending invite.

@carothersc please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

danielskatz commented 4 years ago

@whedon re-invite @carothersc as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@carothersc please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

artgoldberg commented 4 years ago

Hello @danielskatz

I'm beginning to wonder whether Prof. Carothers (@carothersc) will review this paper. I can expand my list of potential reviewers if you would like.

Arthur

danielskatz commented 4 years ago

I was in email contact with @carothersc last week, and he said he should be able to do it soon

artgoldberg commented 4 years ago

Thanks @danielskatz

carothersc-zz commented 4 years ago

Hi all - given that Elsa has confirmed running the software on her side do I need to do the same ? Also, can someone confirm that "pip" won't do anything bad to local "conda" repos?

Thanks everyone!, Chris

danielskatz commented 4 years ago

We expect the reviewers to be independent in terms of investigation, though they may work together in documenting problems and suggesting solutions.

danielskatz commented 4 years ago

Re pip, I suggest the safest thing is to create a new conda environment for this review

artgoldberg commented 4 years ago

@carothersc another approach that would not risk harming your Python computing environment would be to run DE Sim's tests in a Docker image, as I describe in this comment.

gonsie commented 4 years ago

I have completed my review and I approve this paper for acceptance.

I do think there is room for improvement with the model examples given. They are given as complete models, with no description of the process for creating a model. There are a lot of python features that model developers can take advantage of, but it may be hard to discover these without direct support from the de_sim team. Other than that, everything looks great.

artgoldberg commented 4 years ago

Thanks @gonsie . Can DE-Sim issue #55 be closed?

Arthur

danielskatz commented 4 years ago

Thanks @gonsie - I'm a little confused, since you both say "I approve this paper for acceptance" and you have checked all the review boxes, but you also have opened issues. Does this mean that these issues are optional to you: suggestions but not requirements?

gonsie commented 4 years ago

@danielskatz yes. The issues are optional. They are small issues that need not be fixed for my acceptance.

artgoldberg commented 4 years ago

Hello @danielskatz

If Chris is not able to review our paper, perhaps another reviewer could step in. Among the other potential reviewers I recommended in the PRE REVIEW, I would suggest Robert Clayton Blake. He has the appropriate discrete-event simulation and software expertise, and has, in general, been responsive in recent years.

Regards Arthur

danielskatz commented 4 years ago

@carothersc - can you give us a firm date here by when you can do this so we know if we should bring in another reviewer instead of you?

danielskatz commented 4 years ago

I have email from @carothersc that he is not able to complete this due to teaching responsibilities

danielskatz commented 4 years ago

👋 @rblake-llnl - would you be willing to review this submission for JOSS? We carry out our checklist-driven reviews here in GitHub issues and follow these guidelines: https://joss.readthedocs.io/en/latest/review_criteria.html

danielskatz commented 4 years ago

(and we already have one complete review, so I hope nothing major will come up)

danielskatz commented 4 years ago

👋 @yadudoc - would you be willing to review this submission for JOSS? We carry out our checklist-driven reviews here in GitHub issues and follow these guidelines: https://joss.readthedocs.io/en/latest/review_criteria.html

yadudoc commented 4 years ago

@danielskatz, I can review.

danielskatz commented 4 years ago

Thanks @yadudoc - I'm going to create a checklist for you, and invite you. You will need to accept the invitation, then you can work through the checklist items. You should look at the first couple of issues above regarding notifications as well - let me know what else I can do to help you.

danielskatz commented 4 years ago

@whedon add @yadudoc as reviewer

whedon commented 4 years ago

OK, @yadudoc is now a reviewer

danielskatz commented 4 years ago

@whedon re-invite @yadudoc as reviewer

whedon commented 4 years ago

The reviewer already has a pending invite.

@yadudoc please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

danielskatz commented 4 years ago

@yadudoc - you now have a checklist 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 #2685 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 (@danielskatz) if you have any questions/concerns.

yadudoc commented 4 years ago

@artgoldberg, I have completed my review, I'm posting my notes below.

The DE-Sim repo appears to be in good order. It is well documented, and with the contributing guide, I was able to install, run tests and do some tinkering on my laptop without issues. The one error I found, is listed as a github issue.

The manuscript is written well and clearly lays out the problem space, the motivation, and the advantages offered by DE-Sim. The Jupyter notebooks complement the documentation well and serve well to explain how the software could be used. I have added a recommendation to expand on tool integration, a key feature described in the manuscript.

Required issues opened:

Optional issues opened:

danielskatz commented 4 years ago

@yadudoc - are you holding off on the final checkmark in your review until KarrLab/de_sim#63 is resolved? Or is there something else?

yadudoc commented 4 years ago

@danielskatz That issue(https://github.com/KarrLab/de_sim/issues/63) is the only blocker from my end. Apart from this one item, I would recommend accepting this paper.

artgoldberg commented 4 years ago

I appreciate your feedback @yadudoc. Please excuse my slow response. I've been out campaigning for Biden in PA. I'll investigate your issues, especially #63 soon.

Arthur

yadudoc commented 4 years ago

Hi @danielskatz, the issue (https://github.com/KarrLab/de_sim/issues/63) with the tests is now resolved. I recommend this paper for acceptance.

danielskatz commented 4 years ago

Thanks @yadudoc

danielskatz commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

danielskatz commented 4 years ago

👋 @artgoldberg - At this point I'm going to proofread the paper

danielskatz commented 4 years ago

@whedon check references

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

OK DOIs

- 10.1145/2901378.2901402 is OK
- 10.1177/003754978704900506 is OK
- 10.1109/DATE.2001.915002 is OK
- 10.1109/WSC.2003.1261534 is OK
- 10.1109/PADS.2000.847144 is OK
- 10.1016/j.copbio.2017.12.013 is OK
- 10.25080/majora-92bf1922-00a is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1016/j.mib.2015.06.004 is OK
- 10.1016/j.cell.2012.05.044 is OK
- 10.1109/wodes.2006.382398 is OK
- 10.1109/wsc.2005.1574302 is OK

MISSING DOIs

- None

INVALID DOIs

- None
danielskatz commented 4 years ago

👋 @artgoldberg - this all looks good to me. At this point could you:

I can then move forward with accepting the submission.