search

openjournals / joss-reviews

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

[REVIEW]: BASICO: A simplified python interface to COPASI #5553

Closed editorialbot closed 10 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@fbergmann<!--end-author-handle-- (Frank T. Bergmann) Repository: https://github.com/copasi/basico Branch with paper.md (empty if default branch): joss-submission Version: v0.52 Editor: !--editor-->@marcosvital<!--end-editor-- Reviewers: @ayush9pandey, @jguhlin, @darogan Archive: 10.5281/zenodo.8189386

Status

status

Status badge code:

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

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

@ayush9pandey & @jguhlin & @darogan, 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 @marcosvital 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 @ayush9pandey

📝 Checklist for @jguhlin

📝 Checklist for @darogan

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.24 s (514.9 files/s, 259359.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
XML                             21            141             18          27784
Python                          40           4086           4224           9286
YAML                            25             12             15            608
Jupyter Notebook                24              0          14939            568
Markdown                         4             61              0            164
TeX                              1              9              0            110
reStructuredText                 7            113            179             99
JSON                             1              0              0             31
make                             1              3              3              6
-------------------------------------------------------------------------------
SUM:                           124           4425          19378          38656
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 828

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

OK DOIs

- 10.1016/j.jbiotec.2017.06.1200 is OK
- 10.1093/bioinformatics/btg015 is OK
- 10.15252/msb.20199110 is OK
- 10.1002/psp4.3 is OK
- 10.1093/bioinformatics/bth200 is OK

MISSING DOIs

- None

INVALID DOIs

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

marcosvital commented 1 year ago

Dear @ayush9pandey, @jguhlin and @darogan, thank you again for accepting review this submission for JOSS. The reviewing process is checklist based, and instructions were already posted above by the editorial bot - but let me know if you need any assistance, ok? Also, you can tag @fbergmann if you have specific questions about the manuscript.

ayush9pandey commented 1 year ago

Review checklist for @ayush9pandey

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

jguhlin commented 1 year ago

Review checklist for @jguhlin

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

darogan commented 1 year ago

Review checklist for @darogan

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

ayush9pandey commented 1 year ago

Thanks so much for developing BASICO @fbergmann and @marcosvital for the review invitation. My review is below:

BASICO is a Python extension for COPASI, one of the most commonly used biological modeling software. Since COPASI is a GUI-based software, it is not easy to integrate COPASI in modeling and analysis pipelines. So, the development of BASICO is a welcome development. The features in BASICO are powerful and will advance the field of systems biology. The package has an extensive documentation and many helpful features.

I have the following recommendations to improve the paper and the software:

Paper:

  1. The summary ends with "However, sometimes a more automated approach is necessary for example for model13 identification." I would suggest that the motivation for BASICO is described a bit more, because having a Python interface is not just limited to model identification.
  2. It is customary to capitalize "lyapunov", same for "python".
  3. The statement of need talks about COPASI, but beyond COPASI, there are many software and pipelines that have been developed by systems biologists and synthetic biologists which serve similar purpose to that of BASICO. Currently, there is no description or discussion about how BASICO might provide new/unique features in the Python-realm of biological system modeling. It might also help motivate the need for BASICO.
  4. I'd recommend citing the COMBINE, SED-ML, and the Pandas package.
  5. The features and functionality is described in a rather informal way - "we started by implementing X, then people asked for more things so we implemented Y and Z". In my opinion, it would be better if this read more like "X and Y are core tools of BASICO. But, modelers and scientists also need to use Y and Z, so BASICO provides A, B, and C"...or something similar to what is usual in academic papers.
  6. "Profile likelihood plots" point to the documentation of the software. It might be a better reading if it cites the kinds of parameter estimation algorithm that is being used. What are the supported algorithms?
  7. I would have liked to read more about the categories of features that are in COPASI but not in BASICO.
  8. In general, the writing could be more crisp. Some sentences are too long for my taste, and citations to existing tools and research can be added.
  9. Are there any scientific projects where BASICO has been used? It would make the paper much stronger if any particular use case could be highlighted.

Code/Github/Documentation:

  1. The Google Colab notebooks are not self-sufficient. For example, !pip install copasi-basico could be added to the first cell so that the code below it runs.
  2. In Creating_a_simple_model.ipynb Google colab, the plots Y-axis labels are missing and some legends are missing.
  3. When running the stochastic simulation in the above notebook, I got an error about "no output file defined" (https://github.com/copasi/basico/issues/35).
  4. The notebook on "Working_with_Annotations" also gave an error (https://github.com/copasi/basico/issues/34) and ipyparallel has a minor one too (https://github.com/copasi/basico/issues/36)
  5. The central claim for BASICO is that it is difficult for users to use the automatically generated Python bindings. So, it is imperative that all of the examples and notebooks start from the ground up and do not make too many references to internal COPASI structures. This is crucial to ensure that BASICO is widely adopted easily, even by people who might not know much about COPASI. I would recommend refactoring of the examples keeping this in mind. EDIT: On further reading, I do see that many examples do have details in the notebooks, so feel free to ignore this.
  6. A potential concern with parameter scans and parameter identification is the time complexity. It would be interesting to see how BASICO compares with the existing parameter inference software in time complexity and how much it improves (if at all) as compared with the COPASI GUI. The ability to parallelize is wonderful, so it can also be highlighted.
  7. I haven't used the Artistic License before, but does the BASICO copyright belong to the Perl foundation? I believe it is common to write the name of the organization or the person at the top who owns the copyright.
  8. The code coverage, testing, and documentation are great! However, I did run into a couple errors on running pytest on the repository (https://github.com/copasi/basico/issues/37)
marcosvital commented 1 year ago

Thank you for your detailed comments, @ayush9pandey. @fbergmann, let us know if you were able to look and work into those suggestions, ok?

jguhlin commented 1 year ago

@editorialbot generate pdf

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:

fbergmann commented 1 year ago

@editorialbot generate pdf

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:

fbergmann commented 1 year ago

@editorialbot generate pdf

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:

fbergmann commented 1 year ago

@editorialbot generate pdf

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:

fbergmann commented 1 year ago

@marcosvital I finally added the suggestions. Thank you so much to @ayush9pandey for a thorough review. I've tried to reply to all points and applied them to the paper.

Paper:

The summary ends with "However, sometimes a more automated approach is necessary for example for model identification." I would suggest that the motivation for BASICO is described a bit more, because having a Python interface is not just limited to model identification.

You are of course correct, i've changed the sentence to read:

By using a scripting language in conjunction with 
COPASI, scientists can automate tasks such as parameter estimation, sensitivity analysis, 
optimization, and model fitting. They can also create custom scripts to perform complex 
operations or analyses that are not available in COPASI's GUI, such as  model identification.
Scripting languages can also facilitate the communication. documentation and reproducibility
of computational models, by allowing scientists to share their code and results with others.

It is customary to capitalize "lyapunov", same for "python".

Thank you, the capitalizations have been changed

The statement of need talks about COPASI, but beyond COPASI, there are many software and pipelines that have been developed by systems biologists and synthetic biologists which serve similar purpose to that of BASICO. Currently, there is no description or discussion about how BASICO might provide new/unique features in the Python-realm of biological system modeling. It might also help motivate the need for BASICO.

The way I see it, that with basico, the analysis methods of COPASI become readily accessible outside of the graphical user interface of COPASI. Using basico in conjunction with other packages / pipelines might give rise to new functionality, or at least enhance other systems. I've tried to clarify this by mentioning pyABC and the mendes reproducibility study.

I'd recommend citing the COMBINE, SED-ML, and the Pandas package.

Done. I'm not sure i'm happy with the formulation for the COMBINE / SED-ML packages since they are transient dependencies as they are compiled into the python-copasi package.

The features and functionality is described in a rather informal way - "we started by implementing X, then people asked for more things so we implemented Y and Z". In my opinion, it would be better if this read more like "X and Y are core tools of BASICO. But, modelers and scientists also need to use Y and Z, so BASICO provides A, B, and C"...or something similar to what is usual in academic papers.

I have tried to reformulate

"Profile likelihood plots" point to the documentation of the software. It might be a better reading if it cites the kinds of parameter estimation algorithm that is being used. What are the supported algorithms?

The mechanism in general should be used from a good fit obtained (with any available algorithm). From there a local optimization algorithm should be used. Any of them will work for this. It is problem dependent which one is best. As far as the implementation is concerned currently we expose Hooke & Jeeves and Levenberg Marquardt, but more can be added.

I've tried to clarify the section, i didn't provide that initially since i thought the methods should not be described directly.

I would have liked to read more about the categories of features that are in COPASI but not in BASICO.

I would argue that since BASICO builds on top of the SWIG generated language bindings, in theory all analysis methods of COPASI are captured in BASICO. It is just that as users actually try to use them that some simplifications are needed. For example there is no explicit support in Basico for the COPASI tasks Cross section, Time Scale Analysis or Linear Noise Approximation. But if a file using those features that was prepared beforehand using the COPASI user interface those could still be run.

In general, the writing could be more crisp. Some sentences are too long for my taste, and citations to existing tools and research can be added.

I've added links and research references. I'm happy to reformulate anything that is unclear.

Are there any scientific projects where BASICO has been used? It would make the paper much stronger if any particular use case could be highlighted.

It was the initial motivation for me to publish the JOSS submission since i was asked for references to BASICO so it could be used. I'm aware that two publications using basico have been published so far:

I've mentioned those packages in the text.

Code/Github/Documentation:

The Google Colab notebooks are not self-sufficient. For example, !pip install copasi-basico could be added to the first cell so that the code below it runs.

I've added an installation line to the beginning of the landing page for colab and mybinder:

https://colab.research.google.com/github/copasi/basico/blob/master/docs/notebooks/index_colab.ipynb

In Creating_a_simple_model.ipynb Google colab, the plots Y-axis labels are missing and some legends are missing.

I've modified the example to show how y labels could be added (that are not generated by default):

https://colab.research.google.com/github/copasi/basico/blob/master/docs/notebooks/Creating_a_simple_model.ipynb

However, aparently there seems to be an issue with COLAB that some environments might have an older matplotlib version, but as in the SO article described: 

import matplotlib.pyplot as plt
!pip install --upgrade matplotlib

sorts this issue. I dont think i should make that a general thing in the notebook.

When running the stochastic simulation in the above notebook, I got an error about "no output file defined" (JOSS Review: No output file defined in stochastic simulation copasi/basico#35). The notebook on "Working_with_Annotations" also gave an error (JOSS Review: Error in Working_with_Annotations ipynb copasi/basico#34) and ipyparallel has a minor one too (JOSS Review: Errros in Working_with_ipyparallel copasi/basico#36)

Thank you for filing those issues. They have been fixed in newer basico versions.

The central claim for BASICO is that it is difficult for users to use the automatically generated Python bindings. So, it is imperative that all of the examples and notebooks start from the ground up and do not make too many references to internal COPASI structures. This is crucial to ensure that BASICO is widely adopted easily, even by people who might not know much about COPASI. I would recommend refactoring of the examples keeping this in mind. EDIT: On further reading, I do see that many examples do have details in the notebooks, so feel free to ignore this.

I always strive to show at least one example for using new features. I'm happy to refactor specific things when being made aware of it. I went through all the examples to ensure they work. If you see specific things that are too awkward to use I'm happy to change more.

A potential concern with parameter scans and parameter identification is the time complexity. It would be interesting to see how BASICO compares with the existing parameter inference software in time complexity and how much it improves (if at all) as compared with the COPASI GUI. The ability to parallelize is wonderful, so it can also be highlighted.

Certainly those parameter scans can take a long time. That is why in the implementation in basico the generation of COPASI files, can be separated from the processing and plotting. So those files could be generated with basico, then transferred to a cluster and computed there, and later on the plots could be generated from obtained results with basico. It is just for convenience, that basico can process them in parallel and plot them at the same time. Compared to running the identifiability analysis in the GUI: one would have to manually create 2 scan models per parameter to be estimated, then run each of those models and later on post process the results.

I haven't used the Artistic License before, but does the BASICO copyright belong to the Perl foundation? I believe it is common to write the name of the organization or the person at the top who owns the copyright.

It is the license used by the main COPASI project. Since so much of the code stems from COPASI. If this is a hindrance to publication I'm happy to change it to the BSD license.

The code coverage, testing, and documentation are great! However, I did run into a couple errors on running pytest on the repository (JOSS Review: Errors with pytest copasi/basico#37)

I have clarified in the readme file how to run the tests.

ayush9pandey commented 1 year ago

Thanks @fbergmann for all the changes! I don't have any more comments. I recommend the publication of BASICO to JOSS.

jguhlin commented 1 year ago

Hello. My review is short. Installing the software on different operating systems was no problem; the examples from the repository ran with no issues, and the documentation is easy to read and follow. I did not test the jupyter notebooks but only the command-line examples Especially with the comments and changes above, I believe only minor textual edits are required.

Minor text changes: Pg 2 line 70. "Running simulations are a core feature of COPASI, so we started We started BASICO with implementing time course simulations and steady state analysis"

"We started" is there twice.

Pg 2 line 78. "This is another example, that would have been quite cumbersome to do without basico" BASICO should be capitalized.

Pg 2. Line 51. "As scripting module BASICO lends itself for constructing large networks, as is for example done51 in the reproducibility study in (Mendes, 2023)." Remove parentheses around citation (but otherwise keep as citation), if possible.

Very minor change: "At the same time its broad range of tasks, makes it attractive for more advanced computational biologists" to: "At the same time, its broad range of tasks makes it attractive for more advanced computational biologists"

Just moving the comma.

Please let me know if any of those are difficult to achieve. I have no other notes, well done on the library!

fbergmann commented 1 year ago

@editorialbot generate pdf

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:

fbergmann commented 1 year ago

@editorialbot generate pdf

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:

fbergmann commented 1 year ago

Thank you very much for your review @jguhlin, I've addressed all the issues as follows:

Pg 2 line 70. "Running simulations are a core feature of COPASI, so we started We started BASICO with implementing time course simulations and steady state analysis" "We started" is there twice.

thank you for catching that

Pg 2 line 78. "This is another example, that would have been quite cumbersome to do without basico" BASICO should be capitalized.

Indeed!

Pg 2. Line 51. "As scripting module BASICO lends itself for constructing large networks, as is for example done in the reproducibility study in (Mendes, 2023)." Remove parentheses around citation (but otherwise keep as citation), if possible.

Indeed this looks much better without the parentheses using the other reference style this will appear as "... Mendes (2023)."

Very minor change: "At the same time its broad range of tasks, makes it attractive for more advanced computational biologists" to: "At the same time, its broad range of tasks makes it attractive for more advanced computational biologists"

Just moving the comma.

Thank you again!

marcosvital commented 1 year ago

Thank you for all the detailed review and comments, @ayush9pandey and @jguhlin. I see that both of you already checked all the checklist boxes, so I'll tag you and let you know if we happen to need any other feedback from you.

@darogan, since the submission already went through some review, please check the most recent version, ok?

marcosvital commented 1 year ago

@editorialbot generate pdf

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:

marcosvital commented 11 months ago

@fbergmann, I'm sorry this is taking a while to continue. Right now I'm trying to reach our third reviewer by email, and will let you know soon how are we going to proceed, ok?

fbergmann commented 11 months ago

@marcosvital Thank you

darogan commented 11 months ago

My review @fbergmann @marcosvital for BASICO

BASICO provides a python interface to COPASI enabling the more automated and high-throughput analyses. This is a very welcome endeavour and should open up COPASI to a wider audience.

Paper: I have some minor comments to the text

  1. ln25 change mathematical backgrounds to underlying mathematical concepts
  2. Add a brief description of SBML (what is stands for etc)
  3. ln47 readily be integrated
  4. ln50 done -> achieved
  5. ln51 as a scripting
  6. ln60 SED-ML would aid the reader to define what this stands for
  7. ln73 test it a little informal. Suggest updating to something on the lines of "Feature requests from the community led us to add further ..."
  8. ln76 Suggest changing to a more formal style e.g. "The most recent additions include ..."

Code: I was able to run though several examples using Jupyter with ease. Author should be commended on a simple installation and excellent documentation. I didn't try the google colab options, so can't comment on ease of use here

darogan commented 11 months ago

@marcosvital @fbergmann apologies for the delays in my review

fbergmann commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

fbergmann commented 11 months ago

@darogan, thank you very much for your review, I've applied all your comments.

marcosvital commented 11 months ago

Thank you for your contributions to this submission, @darogan.

marcosvital commented 11 months ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.jbiotec.2017.06.1200 is OK
- 10.1093/bioinformatics/btg015 is OK
- 10.15252/msb.20199110 is OK
- 10.1002/psp4.3 is OK
- 10.1093/bioinformatics/bth200 is OK
- 10.21105/joss.04304 is OK
- 10.48550/arXiv.2307.08452 is OK
- 10.3389/fcell.2023.1201673 is OK
- 10.5281/zenodo.8092754 is OK
- 10.5281/zenodo.7408732 is OK
- 10.5281/zenodo.6619620 is OK
- 10.1186/s12859-014-0369-z is OK
- 10.1016/j.biosystems.2012.09.003 is OK

MISSING DOIs

- None

INVALID DOIs

- None
marcosvital commented 11 months ago

@editorialbot set v0.52 as version

editorialbot commented 11 months ago

Done! version is now v0.52

marcosvital commented 11 months ago

@fbergmann it seems that we are almos ready to finish. Please take a look at the last version of the manuscript, and let me know if everything is ok.

Also, if you already didn't do this: you will need to archive the last release of the package (on Zenodo, figshare, or other of your choice) and share the archive DOI here.

fbergmann commented 11 months ago

@marcosvital I'm happy with the final version. And I do have the automatic zenodo integration going, that will create a release package for each release. The last one being currently https://zenodo.org/record/8189386. Please let me know if I can do anything else to expedite.

marcosvital commented 11 months ago

@editorialbot set 10.5281/zenodo.8189386 as archive

editorialbot commented 11 months ago

Done! archive is now 10.5281/zenodo.8189386

marcosvital commented 11 months ago

@editorialbot recommend-accept

editorialbot commented 11 months ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 11 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.jbiotec.2017.06.1200 is OK
- 10.1093/bioinformatics/btg015 is OK
- 10.15252/msb.20199110 is OK
- 10.1002/psp4.3 is OK
- 10.1093/bioinformatics/bth200 is OK
- 10.21105/joss.04304 is OK
- 10.48550/arXiv.2307.08452 is OK
- 10.3389/fcell.2023.1201673 is OK
- 10.5281/zenodo.8092754 is OK
- 10.5281/zenodo.7408732 is OK
- 10.5281/zenodo.6619620 is OK
- 10.1186/s12859-014-0369-z is OK
- 10.1016/j.biosystems.2012.09.003 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 11 months ago

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

Check final proof :point_right::page_facing_up: Download article

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/4682, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept