[REVIEW]: Simframe: A Python Framework for Scientific Simulations

whedon commented 3 years ago

Submitting author: @stammler (Sebastian Markus Stammler) Repository: https://github.com/stammler/simframe Version: 1.0.0 Editor: @taless474 Reviewer: @schruste, @lucaferranti Archive: 10.5281/zenodo.5785575

: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 badge code:

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

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

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

Make sure you're logged in to your GitHub account
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 @taless474 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 @schruste

✨ 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

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

Code of Conduct

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

General checks

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

Functionality

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

Documentation

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

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[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?

Review checklist for @lucaferranti

✨ 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

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

Code of Conduct

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

General checks

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

Functionality

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

Documentation

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

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[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?

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @schruste, @lucaferranti 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:

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

watching

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

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

OK DOIs

- None

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 3 years ago

Wordcount for paper.md is 860

whedon commented 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.16 s (517.9 files/s, 269048.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          67           1050           1650           3056
Jupyter Notebook                 7              0          35289            782
Markdown                         2             37              0             85
DOS Batch                        1              8              1             26
reStructuredText                 2             15             23             14
YAML                             1              4              7              9
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            81           1118          36977           3981
-------------------------------------------------------------------------------

Statistical information for the repository 'cc3f6bc1a085c727fbe7d7c7' was
gathered on 2021/11/02.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Sebastian Stammler             108          9221           3571           98.89
birnstiel                        3           127             17            1.11

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
Sebastian Stammler         5661           61.4          7.9                6.41
Til Birnstiel                95          100.0         12.3               24.21

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:

stammler commented 3 years ago

@taless474: A quick question. We just noticed that whedon did not assign the second reviewer schruste to this review. Is this correct?

taless474 commented 2 years ago

@taless474: A quick question. We just noticed that whedon did not assign the second reviewer schruste to this review. Is this correct?

@stammler, You are right. But I guess the issue is correct and both @lucaferranti and @schruste are the reviewers. Thank you for noticing it.

whedon commented 2 years ago

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

whedon commented 2 years ago

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

schruste commented 2 years ago

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

Hi. Sorry, This got lost a bit. I've put this on my schedule for next week.

lucaferranti commented 2 years ago

apologies for the delay with this from my side. I'm planning to post my review during the weekend

schruste commented 2 years ago

@taless474 @whedon : It seems the invitation is no longer valid, could you please re-invite me?

schruste commented 2 years ago

@stammler: I don't have the rights to set the check marks yet (solved now), so I will start with some direct comments:

I think the simframe package is an interest package with a well-defined (small) set of features which seem to be however quite useful. Here some further remarks:

It seems that you have not community guidelines, yet. Perhaps you can add them to your README.md ?
In paper.md you have "One one hand". I guess this should be "On the one hand"
Some basic explanations of the scope of the software that you have in paper.md would also be appropriate to put in the README.md.
Especially the ties that you have with NumPy should be mentioned early on. Because if I get this correctly you "only" allow for objects/vectors from the NumPy universe. In principle I could easily imagine cases where I would like to use your package, but have my own data types for my state vectors, etc.. I think it is not too much of a restriction that you have here, but it would be nice to clarify this early on.
Have you thought about setting up a small binder? This should be pretty easily set up and would even increase accessibility.

lucaferranti commented 2 years ago

@whedon generate pdf

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

lucaferranti commented 2 years ago

@whedon check references

whedon commented 2 years ago

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

OK DOIs

- None

MISSING DOIs

- None

INVALID DOIs

- None

lucaferranti commented 2 years ago

Hi @stammler , very nice work, here are some comments:

Paper

[x] comparison to other softwares: the paper says that there are plenty of ODE solvers packages (which is definitely true), it would be good to mention a few and give a more detailed comparison of simframe to other similar packages, i.e. what features simframe has that other mainstream packages haven't and vice versa.
[x] references: It is quite strange to me that the paper has no references at all. For example, the integration schemes section could greatly benefit for some references, as they would guide the reader in finding more details about the schemes. Addressing the previous point would also probably lead to some references.

Documentation

[x] Maybe a little surprisingly, I could run all notebooks in the examples folder except the first one, it gets stuck at the fourth last cell sim_cont.run() and then the kernel dies. Is that example somehow particularly computationally expensive? If it is, it would be good to mention it explicitly in the notebook. I'll retry this later to verify it wasn't a strange glitch on my side.
[x] (minor) It would be nice to add in each example in the docs a "download the notebook" button and/or a "try it on binder" button if you setup binder as suggested by the other reviewer.
[x] automated tests: I would strongly suggest to setup a github actions (or travis, or your favourite CI tool) to automatically run the tests whenever someone pushes (to master or through PR), this is mainly to make sure nobody accidentally breaks things while contributing. Github should have plenty of workflow-templates for python packages, you can find those from the actions tab. It would also be nice to have some coverage tool such as codecov or coveralls
[x] contribution guidelines: As pointed out by the other reviewer, contribution guidelines are missing. Github has a cheatsheet of "good social practices" under the repo insights > community. There you can find a few tips to write contributions guidelines.

taless474 commented 2 years ago

@whedon re-invite @schruste as reviewer

whedon commented 2 years ago

OK, the reviewer has been re-invited.

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

stammler commented 2 years ago

Dear @schruste and @lucaferranti!

Thank you for your great comments!

We added contribution guidelines, templates for bug report and feature request issues, and a template for pull requests. We furthermore added a small explanation about the scope of the software in the README.md.

We changed the license to the more permissive BSD 3-Clause License.

We clarified in the paper and the README.md that simframe can only handle data, that can be stored within NumPy arrays. We furthermore added references to the used packages and the integration schemes.

All example notebooks of the documentation have now a button to launch them on binder. Since the first notebook works fine on Binder and on Readthedocs for building the documentation, we assume that your reported problem, @lucaferranti, is local on your setup for now. Please let us know if the problem persists.

We added automated unit tests to Github Actions on pull requests and merges to master.

We would like to refrain from directly comparing simframe to existing ODE solvers. The point of simframe is not to provide solvers, but to provide an infrastructure to run scientific simulations including data structures and input/output methods. The simple integration schemes that we provide by default are only a bonus, such that simframe can be used as is. Existing solvers like solve_ivp or odeint, on the other hand, can be used as integration scheme within simframe. We clarified this in the paper.

Thanks again! And please let us know if you have more comments.

stammler commented 2 years ago

@whedon generate pdf

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

lucaferranti commented 2 years ago

hi @stammler 👋 ,

I'll have a closer look today or tomorrow, overall it looks very good!

One quick small comment: I noticed your workflow runs only on ubuntu, have you considering running it also on windows and macos? I think it would make the testing more robust to possible small OS dependent bugs

stammler commented 2 years ago

Thanks! We added Windows and MacOS to the workflow.

lucaferranti commented 2 years ago

@whedon check references

whedon commented 2 years ago

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

OK DOIs

- None

MISSING DOIs

- 10.25080/majora-ebaa42b7-00d may be a valid DOI for title: Building a Framework for Predictive Science

INVALID DOIs

- None

lucaferranti commented 2 years ago

@stammler @taless474 after the latest changes I think this submission is good to go. There might be a missing doi in the references, but after that is checked I think the paper is ready for publication.

stammler commented 2 years ago

@whedon check references

whedon commented 2 years ago

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

OK DOIs

- 10.1007/978-3-540-78862-1 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.25080/majora-ebaa42b7-00d is OK

MISSING DOIs

- None

INVALID DOIs

- None

stammler commented 2 years ago

We added the missing DOIs.

stammler commented 2 years ago

A quick question about the procedure: Is now the point where we should upload it to Zenodo and provide a DOI or is there something else that has to be done first?

taless474 commented 2 years ago

A quick question about the procedure: Is now the point where we should upload it to Zenodo and provide a DOI or is there something else that has to be done first?

Hi @stammler, yes please go ahead and upload it. You can refer to links in here. @lucaferranti and @schruste, thank you for your revisions, good job 👍

stammler commented 2 years ago

@taless474: the DOI is 10.5281/zenodo.5785575. The version number is 1.0.0

@lucaferranti & @schruste: Thank you for your great comments!

birnstiel commented 2 years ago

@taless474: sorry to bother you again, but @stammler uploaded the paper and posted the DOI above. Is there anything else that we need to do to move forward? Thanks everyone for the reviews and the editorial support!

taless474 commented 2 years ago

@whedon generate pdf

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

taless474 commented 2 years ago

@birnstiel thanks for reminding me here. Everything looks good to me.

taless474 commented 2 years ago

@whedon set 10.5281/zenodo.5785575 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.5785575 is the archive.

taless474 commented 2 years ago

@whedon check references

whedon commented 2 years ago

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

OK DOIs

- 10.1007/978-3-540-78862-1 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.25080/majora-ebaa42b7-00d is OK

MISSING DOIs

- None

INVALID DOIs

- None

taless474 commented 2 years ago

@whedon recommend-accept

whedon commented 2 years ago

Attempting dry run of processing paper acceptance...

whedon commented 2 years ago

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

OK DOIs

- 10.1007/978-3-540-78862-1 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.25080/majora-ebaa42b7-00d is OK

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 2 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2875

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2875, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true

stammler commented 2 years ago

Not sure if this matters. But the latest version is 1.0.0, while the top comment of the issue still says 0.5.4

danielskatz commented 2 years ago

@whedon set 1.0.0 as version

whedon commented 2 years ago

OK. 1.0.0 is the version.

danielskatz commented 2 years ago

@stammler - I'm the next step in the process. I'll proofread it in the next couple of hours, and either let you know if any changes need to be made, or will proceed to publication.

openjournals / joss-reviews