openjournals / joss-reviews

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

[REVIEW]: Chi: A Python package for treatment response modelling #5925

Closed editorialbot closed 8 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@DavAug<!--end-author-handle-- (David Augustin) Repository: https://github.com/DavAug/chi Branch with paper.md (empty if default branch): joss-publication Version: 1.0.0 Editor: !--editor-->@ppxasjsm<!--end-editor-- Reviewers: @shahmoradi, @ns-rse Archive: 10.5281/zenodo.10510572

Status

status

Status badge code:

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

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

@shahmoradi, 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 @ppxasjsm 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 @ns-rse

📝 Checklist for @shahmoradi

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.73 s (130.1 files/s, 54335.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          39           5856          10146          20465
HTML                            10              0              0            710
reStructuredText                26            345            423            678
XML                              5             15              0            401
YAML                             8             43             11            228
TeX                              1             14              0            152
Markdown                         3             36              0            108
DOS Batch                        1              8              1             27
make                             1              4              6             10
CSS                              1              0              1              3
-------------------------------------------------------------------------------
SUM:                            95           6321          10588          22782
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 494

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

OK DOIs

- 10.1371/journal.pcbi.1011135 is OK
- 10.1038/nrd.2017.244 is OK
- 10.1101/2023.07.31.551404 is OK
- 10.5334/jors.252 is OK

MISSING DOIs

- 10.1038/psp.2013.24 may be a valid DOI for title: Modeling and simulation workbench for NONMEM: tutorial on Pirana, PsN, and Xpose
- 10.1007/s11095-021-03065-1 may be a valid DOI for title: Scipion PKPD: an Open-Source Platform for Biopharmaceutics, Pharmacokinetics and Pharmacodynamics Data Analysis
- 10.1101/2020.11.28.402297 may be a valid DOI for title: Accelerated predictive healthcare analytics with pumas, a high performance pharmaceutical modeling and simulation platform
- 10.1093/aje/kwt245 may be a valid DOI for title: Maximum likelihood, profile likelihood, and penalized likelihood: a primer
- 10.1007/s11222-013-9416-2 may be a valid DOI for title: Understanding predictive information criteria for Bayesian models
- 10.1101/2022.03.19.483454 may be a valid DOI for title: Treatment response prediction: Is model selection unreliable?

INVALID DOIs

- https://doi.org/10.1016/j.ddtec.2016.11.005 is INVALID because of 'https://doi.org/' prefix
editorialbot commented 1 year ago

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

Kevin-Mattheus-Moerman commented 1 year ago

@ppxasjsm note that over in the pre-review issue @ns-rse said they could help review too.

ppxasjsm commented 1 year ago

Thanks! I somehow missed this. @ns-rse I'll add you as a reviewer, please let me know if you cannot review anymore.

ppxasjsm commented 1 year ago

@editorialbot add @ns-rse as reviewer

editorialbot commented 1 year ago

@ns-rse added to the reviewers list!

ppxasjsm commented 1 year ago

@DavAug could you take a look at the missing DOIs flagged by editorial bot and fix them please?

DavAug commented 1 year ago

@DavAug could you take a look at the missing DOIs flagged by editorial bot and fix them please?

Of course! No problem 😊

ns-rse commented 1 year ago

Review checklist for @ns-rse

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

DavAug commented 1 year ago

@editorialbot generate pdf

DavAug commented 1 year ago

@DavAug could you take a look at the missing DOIs flagged by editorial bot and fix them please?

I fixed the Invalid DOI and manually checked the "Missing" ones. They all link to the correct publication on https://www.doi.org/

editorialbot commented 1 year ago

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

DavAug commented 1 year ago

@editorialbot commands

editorialbot commented 1 year ago

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


# List all available commands
@editorialbot commands

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

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

# Perform checks on the repository
@editorialbot check repository

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

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

# Generates the pdf paper
@editorialbot generate pdf

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers
DavAug commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1371/journal.pcbi.1011135 is OK
- 10.1038/nrd.2017.244 is OK
- 10.1016/j.ddtec.2016.11.005 is OK
- 10.1101/2023.07.31.551404 is OK
- 10.5334/jors.252 is OK

MISSING DOIs

- 10.1038/psp.2013.24 may be a valid DOI for title: Modeling and simulation workbench for NONMEM: tutorial on Pirana, PsN, and Xpose
- 10.1007/s11095-021-03065-1 may be a valid DOI for title: Scipion PKPD: an Open-Source Platform for Biopharmaceutics, Pharmacokinetics and Pharmacodynamics Data Analysis
- 10.1101/2020.11.28.402297 may be a valid DOI for title: Accelerated predictive healthcare analytics with pumas, a high performance pharmaceutical modeling and simulation platform
- 10.1093/aje/kwt245 may be a valid DOI for title: Maximum likelihood, profile likelihood, and penalized likelihood: a primer
- 10.1007/s11222-013-9416-2 may be a valid DOI for title: Understanding predictive information criteria for Bayesian models
- 10.1101/2022.03.19.483454 may be a valid DOI for title: Treatment response prediction: Is model selection unreliable?

INVALID DOIs

- None
DavAug commented 12 months ago

One of the articles that we are referencing just got published, so I am just replacing the reference to the preprint by the reference to the peer-reviewed publication.

DavAug commented 12 months ago

@editorialbot generate pdf

DavAug commented 12 months ago

@editorialbot check references

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

OK DOIs

- 10.1371/journal.pcbi.1011135 is OK
- 10.1038/nrd.2017.244 is OK
- 10.1016/j.ddtec.2016.11.005 is OK
- 10.3389/fphar.2023.1270443 is OK
- 10.5334/jors.252 is OK
- 10.1016/j.pbiomolbio.2015.12.008 is OK

MISSING DOIs

- 10.1038/psp.2013.24 may be a valid DOI for title: Modeling and simulation workbench for NONMEM: tutorial on Pirana, PsN, and Xpose
- 10.1007/s11095-021-03065-1 may be a valid DOI for title: Scipion PKPD: an Open-Source Platform for Biopharmaceutics, Pharmacokinetics and Pharmacodynamics Data Analysis
- 10.1101/2020.11.28.402297 may be a valid DOI for title: Accelerated predictive healthcare analytics with pumas, a high performance pharmaceutical modeling and simulation platform
- 10.1093/aje/kwt245 may be a valid DOI for title: Maximum likelihood, profile likelihood, and penalized likelihood: a primer
- 10.1007/s11222-013-9416-2 may be a valid DOI for title: Understanding predictive information criteria for Bayesian models
- 10.1101/2022.03.19.483454 may be a valid DOI for title: Treatment response prediction: Is model selection unreliable?

INVALID DOIs

- None
editorialbot commented 12 months ago

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

DavAug commented 11 months ago

Hi @ppxasjsm @ns-rse @shahmoradi @Kevin-Mattheus-Moerman

I hope you are all doing well and had a fantastic start to the day :)

I just wanted to kindly follow up on the status of this review, and kindly ask whether there is anything I can do to help you?

Best wishes, David

shahmoradi commented 11 months ago

Yes. I can finish this review by tomorrow night. I have been assigned multiple JOSS review tasks, which have made it super-confusing for me on what is complete and what needs completion. Thanks for your patience.

ns-rse commented 11 months ago

Thanks for the nudge @DavAug I'll have my review finished by the end of the weekend (2023-11-05).

shahmoradi commented 11 months ago

Review checklist for @shahmoradi

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

shahmoradi commented 11 months ago

Thank you for your efforts. I have checkmarked most review items. I have found a few issues highlighted below:

  1. The documentation should ideally start with installation guidelines or pointers to such guidelines (which seem to exist in the readme file of the repository).
  2. The example given in the documentation failed with the following error message:
    
    C:\ProgramData\Anaconda3\lib\site-packages\numba\np\ufunc\decorators.py in <module>
      1 import inspect
      2 
    ----> 3 from numba.np.ufunc import _internal
      4 from numba.np.ufunc.parallel import ParallelUFuncBuilder, ParallelGUFuncBuilder
      5 

SystemError: initialization of _internal failed without raising an exception


Please ensure examples or code snippets in the documentation are independent and self-sufficient.
3. Some of the documentation pages are incomplete and should be either removed or completed. Here is an example [broken page](https://chi.readthedocs.io/en/latest/getting_started/log_likelihood.html). Here is another [broken page](https://chi.readthedocs.io/en/latest/getting_started/log_posterior.html).
4. Ideally, there should also be instructions in the repository to run the tests for everyone, novice or expert. But I do see unit-tests and code-coverage reports which is essential.
5. Could you ensure all the requested items in "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" are in place?
DavAug commented 11 months ago

Thank you for your efforts. I have checkmarked most review items. I have found a few issues highlighted below:

  1. The documentation should ideally start with installation guidelines or pointers to such guidelines (which seem to exist in the readme file of the repository).
  2. The example given in the documentation failed with the following error message:
C:\ProgramData\Anaconda3\lib\site-packages\numba\np\ufunc\decorators.py in <module>
      1 import inspect
      2 
----> 3 from numba.np.ufunc import _internal
      4 from numba.np.ufunc.parallel import ParallelUFuncBuilder, ParallelGUFuncBuilder
      5 

SystemError: initialization of _internal failed without raising an exception

Please ensure examples or code snippets in the documentation are independent and self-sufficient. 3. Some of the documentation pages are incomplete and should be either removed or completed. Here is an example broken page. Here is another broken page. 4. Ideally, there should also be instructions in the repository to run the tests for everyone, novice or expert. But I do see unit-tests and code-coverage reports which is essential. 5. Could you ensure all the requested items in "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" are in place?

Thank you @shahmoradi for your helpful review and comments.

Could you, perhaps, provide some more details what you did that spawned this error message (which environment you are using and how you installed the package)?. I will do my best to fix this issue.

ns-rse commented 11 months ago

Some additional notes I made when reviewing....

Functionality

Installation

Personally I don't use Debian and so under Arch had to pacman -Syu sundials, although I appreciate it can be challenging to cover all use cases, particularly with Linux distributions but perhaps Windows users might benefit from some instructions on how to install external dependencies.

Installation would perhaps benefit from advising the use of a virtual environment to install the package under but not essential.

Performance

Claims the package is easier to use than alternatives but personally I'm not familiar with these and did not have the time to investigate or run comparable analyses with the listed alternatives, so I couldn't judge this aspect.

Documentation

Functionality Documentation

When I looked I found as @shahmoradi did some of the pages aren't complete, but I see you've already acknowledged this and will address it.

Community Guidelines

Whilst there is a Contributing section in the README.md it is sparse on guidelines (e.g. no mention of the fact Flake8 formatting appears to be applied) and details of how to report issues or seek report. This could be expanded guiding people to create issues if they find problems (not everyone is familiar with GitHub nor how to report problems).

In this regard the repository could perhaps benefit from having some Issue Templates for reporting of errors which prompts users to provide certain fields such as versions of the package, its dependencies and Python version, copy and pasting the error message, provide input parameters and so forth. Similarly a Features template would be beneficial.

DavAug commented 11 months ago

Some additional notes I made when reviewing....

Functionality

Installation

Personally I don't use Debian and so under Arch had to pacman -Syu sundials, although I appreciate it can be challenging to cover all use cases, particularly with Linux distributions but perhaps Windows users might benefit from some instructions on how to install external dependencies.

Installation would perhaps benefit from advising the use of a virtual environment to install the package under but not essential.

Performance

Claims the package is easier to use than alternatives but personally I'm not familiar with these and did not have the time to investigate or run comparable analyses with the listed alternatives, so I couldn't judge this aspect.

Documentation

Functionality Documentation

When I looked I found as @shahmoradi did some of the pages aren't complete, but I see you've already acknowledged this and will address it.

Community Guidelines

Whilst there is a Contributing section in the README.md it is sparse on guidelines (e.g. no mention of the fact Flake8 formatting appears to be applied) and details of how to report issues or seek report. This could be expanded guiding people to create issues if they find problems (not everyone is familiar with GitHub nor how to report problems).

In this regard the repository could perhaps benefit from having some Issue Templates for reporting of errors which prompts users to provide certain fields such as versions of the package, its dependencies and Python version, copy and pasting the error message, provide input parameters and so forth. Similarly a Features template would be beneficial.

Thank you @ns-rse , really helpful additions! I am in the process of updating the docs and will address your comments in the next couple of days 😊

ns-rse commented 11 months ago

@DavAug You're welcome.

Certainly not required but ruff is an excellent alternative to Flake8, it covers all the rules Flake8 does, is considerably faster and has a pre-commit hook which can be added and integrated with GitHub Actions via pre-commit.ci. For more on this see Research Software Engineering Sheffield - pre-commit : Protecting your future self.

DavAug commented 10 months ago

@editorialbot generate pdf

editorialbot commented 10 months ago

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

DavAug commented 10 months ago

Hi @ns-rse @shahmoradi @ppxasjsm @Kevin-Mattheus-Moerman

I hope you had a great weekend and thank you for your patience :)

I have completed cleaning up the documentation page and tried to address your comments above. In particular:

  1. I have added install instructions to the landing page of the documentation: https://chi.readthedocs.io . These instructions detail how to install the package using pip on Windows, MacOS and Ubuntu. I chose these operating systems, because I can test the installs on these operating systems with GitHub workflows. (This means I have not included the install instructions that you kindly provided @ns-rse . I hope that is ok).
  2. I have extended the tutorials quite substantially to demonstrate in more detail how to use Chi and to highlight the benefits of using Chi, see Getting started section in the documentation. I have workflows in place that test these tutorials, so the examples should work if chi is correctly installed, at least on the tested operating systems. (My unit test workflows run the tests on Python version 3.9, 3.10 and 3.11.)
  3. I have removed any pointers to tutorials that may be added in the future. So there are no "broken" pages in the documentation left.
  4. I have added an extensive CONTRIBUTING.md which hopefully encourages the community to contribute to Chi and simplifies the onboarding.

The one point that remains unaddressed are your challenges with installing the package, @shahmoradi . Would you, perhaps, be able to try installing the package again following the install instructions on https://chi.readthedocs.io . I hope that this will resolve the problem :)

I hope you have a nice rest of the day and best wishes, David

DavAug commented 10 months ago

Hi and good morning @shahmoradi ,

I just wanted to kindly follow up on the above and ask whether you were able to successfully install Chi now?

I hope you have a nice day and best wishes, David

DavAug commented 10 months ago

Hi all @ppxasjsm @Kevin-Mattheus-Moerman @ns-rse @shahmoradi

I hope you had a great start to the day 😊

I just wanted to kindly follow up on the status of this review?

I hope you have a nice day and best wishes, David

ns-rse commented 10 months ago

Morning @DavAug ,

Thanks again for the nudge. Had a look through the contributing guidelines are a great addition, thanks for adding those. There is a clear statement of need too and so I've ticked off the last couple of items on my check list.

If you've ever time to work on this I can highly recommend pre-commit and pre-commit.ci the later of which integrates well with a GitHub action and catches instances where contributors might not have applied Flake8 locally before commits (I added some links to more information in a previous comment that may be useful).

Have fun and all the best,

@ns-rse

DavAug commented 10 months ago

Hi @shahmoradi @ppxasjsm @Kevin-Mattheus-Moerman

I hope you had a great day so far :)

I just wanted to kindly follow up on this review and kindly ask whether there is anything I can do to facilitate the process?

Best wishes, David

shahmoradi commented 10 months ago

Thanks @DavAug, for your reminder and response. The error still exists, but given your package does not use numba, I take it as an issue for me to resolve. I have checkmarked the rest of the items that you have addressed.

DavAug commented 10 months ago

Thanks @DavAug, for your reminder and response. The error still exists, but given your package does not use numba, I take it as an issue for me to resolve. I have checkmarked the rest of the items that you have addressed.

Super, thank you so much!

DavAug commented 9 months ago

Hi @ppxasjsm @Kevin-Mattheus-Moerman and happy New Year! 😊

I just wanted to kindly follow up on this and ask whether there are any remaining steps on my side to conclude this process, given that both reviewers are now satisfied with the submission?

I hope you have a great day and best wishes, David

ppxasjsm commented 9 months ago

@editorialbot generate pdf

ppxasjsm commented 9 months ago

Hi @DavAug,

Happy new year! My apologies for processing this slowly, but I had taken some extended time off over the holidays.

There are a small number of things you will still have to do to process this submission. Just a word of warning I am still out of office until the 24th Jan, so may be a bit slow here still.

editorialbot commented 9 months ago

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

ppxasjsm commented 9 months ago

@editorialbot check references

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

OK DOIs

- 10.1371/journal.pcbi.1011135 is OK
- 10.1038/nrd.2017.244 is OK
- 10.1016/j.ddtec.2016.11.005 is OK
- 10.3389/fphar.2023.1270443 is OK
- 10.5334/jors.252 is OK
- 10.1016/j.pbiomolbio.2015.12.008 is OK

MISSING DOIs

- 10.1038/psp.2013.24 may be a valid DOI for title: Modeling and simulation workbench for NONMEM: tutorial on Pirana, PsN, and Xpose
- 10.1007/s11095-021-03065-1 may be a valid DOI for title: Scipion PKPD: an Open-Source Platform for Biopharmaceutics, Pharmacokinetics and Pharmacodynamics Data Analysis
- 10.1101/2020.11.28.402297 may be a valid DOI for title: Accelerated predictive healthcare analytics with pumas, a high performance pharmaceutical modeling and simulation platform
- 10.1093/aje/kwt245 may be a valid DOI for title: Maximum likelihood, profile likelihood, and penalized likelihood: a primer
- 10.1007/s11222-013-9416-2 may be a valid DOI for title: Understanding predictive information criteria for Bayesian models
- 10.1101/2022.03.19.483454 may be a valid DOI for title: Treatment response prediction: Is model selection unreliable?

INVALID DOIs

- None
ppxasjsm commented 9 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

ppxasjsm commented 9 months ago

@DavAug, I have created a checklist for us before everything is final. I noticed that some DOIs seem to be missing, for now now could you:

I'll go through the paper and may make small additional editorial suggestions either here or as a PR.