search

openjournals / joss-reviews

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

[REVIEW]: Fastsubtrees: simple and efficient subtrees extractions in Python with applications to NCBI taxonomy #4755

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@ggonnella<!--end-author-handle-- (Giorgio Gonnella) Repository: https://github.com/ggonnella/fastsubtrees Branch with paper.md (empty if default branch): main Version: 2.1 Editor: !--editor-->@kellyrowland<!--end-editor-- Reviewers: @vinisalazar, @KonradHoeffner Archive: 10.5281/zenodo.7312100

Status

status

Status badge code:

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

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

@vinisalazar & @KonradHoeffner, 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 @kellyrowland 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 @KonradHoeffner

📝 Checklist for @vinisalazar

editorialbot commented 2 years 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 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.05 s (993.7 files/s, 62539.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          36            228            322           1642
Markdown                         9            149              0            533
TeX                              1              9              0            115
make                             2             16              8             39
YAML                             1              1              4             18
-------------------------------------------------------------------------------
SUM:                            49            403            334           2347
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 1604

editorialbot commented 2 years ago

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

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

OK DOIs

- 10.1093/nar/gkz246 is OK
- 10.1186/s13104-019-4577-5 is OK
- 10.1093/database/baaa062 is OK

MISSING DOIs

- Errored finding suggestions for "Ntmirror Python package", please try later
- Errored finding suggestions for "Fastsubtrees Python package", please try later
- Errored finding suggestions for "Definition: tree", please try later

INVALID DOIs

- None
KonradHoeffner commented 2 years ago

Review checklist for @KonradHoeffner

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

vinisalazar commented 2 years ago

Review checklist for @vinisalazar

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

KonradHoeffner commented 2 years ago

My comments on the boxes I didn't check:

Functionality

Installation

Installation only works with an undocumented workaround, see Documentation -> installation instructions

Functionality and Performance

I tried to run the fastsubtrees-construct script but it is not clear how exactly to reproduce the tables in the paper. There should be a single parameterless script that runs all the benchmarks. A Dockerfile that installs all necessary python and other dependencies would probably be especially helpful, as it would get around the platform specific dependencies.

Documentation

Installation instructions

Automated tests

There is a test folder with pyttest test scripts but there the tests fail to run, see https://github.com/ggonnella/fastsubtrees/issues/4.

Community guidelines

There are no explicit community guidelines in the README.md but it's a GitHub repository with an issue tracker for reporting issues and seek support, but the way the checklist is written implies this is not enough

Software Paper

References

Other

KonradHoeffner commented 2 years ago

@editorialbot commands

editorialbot commented 2 years ago

Hello @KonradHoeffner, 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
KonradHoeffner commented 2 years ago

@kellyrowland: I am finished with my first review and checked all the boxes I feel comfortable checking. Unfortunately I cannot accept the paper at the current state due to the reasons commented above. However if the authors fix the two acceptance blockers (issues filed at the target repository because of failing installation and failing tests) and address the other comments, this can be changed in my opinion and in that case I can evaluate the claims, which is not possible right now. As this is my first review, I'm not sure how I signal to the editorialbot that I am finished reviewing, there does not seem to be an editorialbot command where I can submit my recommendation, so I hope just writing this comment is enough.

ggonnella commented 2 years ago

Thank you for the reviews. We will address the issues and answer to the comments.

vinisalazar commented 2 years ago

Hi, I checked the first few boxes of the review checklist but I will only be able to resume my review in the next week. So, if requested changes are incorporated by then, I can take them into account for my review.

Thanks V

ggonnella commented 2 years ago

I created a docker as suggested by the reviewer. This allows to run tests, benchmarks and the example application without hassles. I improved the documentation, including the part regarding installation.

I updated the benchmark results in the paper, using values obtained using the Docker mentioned above. In the repository are now scripts, which allow to reproduce the tables of the paper fully automatically.

ggonnella commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

vinisalazar commented 2 years ago

Hi,

I believe that the problem targeted by the software is legitimate and that the applications are solid. So, I'm inclined to accept this paper. However, I agree that there are a number of issues that must be addressed.

I was able to successfully install from both source and using the Docker image, and to run the tests.

The Docker image proved to be the easiest method. Although it did take long to build, once the container was ready, all commands could be executed. Still, I think many users will not be familiar with Docker, so installation from pip or some other package manager is preferable. Here comes in the problem documented in ggonnella/fastsubtrees#3, of the MariaDB Server dependency. I was able to install MariaDB Server locally using brew, but if it's a core dependency of the package, it must be packaged along with it. I think the preferable alternative for packaging fastsubtrees would be conda, specifically the Bioconda channel. Another problem here is that mariadb-server itself does not have a conda distribution (except for this one, which hasn't been updated in 6 years), so one would have to be made for it, or MariaDB would need to be compiled from source in the fastsubtrees recipe build script (harder to do). This would allow one to specify mariadb-server as a dependency and to install fastsubtrees without Docker in a single command.

I insist on this point of installation because I think it's the main barrier for potential users of the package. If that is fixed, it will significantly improve the package accessibility. Additionally, it would be nice to have an additional level of detail on how to install MariaDB Server, or even why it's required, so users find it easier to do it themselves, if they have to.

Here are some other observations which I think would help improve the package:

I don't think it's mandatory to address all of these immediately, but they would certainly contribute to the overall state of the paper. Please let me know if you have any questions.

Kind regards, Vini

ggonnella commented 2 years ago

Thank you very much for your review and constructive comments. I think we can address several of the minor points. I have indeed some questions/comments, regarding the other:

vinisalazar commented 2 years ago

@ggonnella sounds good, if you could separate MariaDB from the overall installation, that would be great. I ran the make benchmarks_all on my laptop and it finished it without any problems, so I think that the Docker image is working well for the comparative benchmarks, for folks who are keen to reproduce that.

  • Actually tests cover all operations, and definitely at least most of the code under tree.py and attribute.py (which contain most of the code), for which the report says "0%".

If that is the case, do you think it would be feasible to run the testing suite plus a coverage report on a GitHub Actions workflow?

Best wishes, Vini

kellyrowland commented 2 years ago

@KonradHoeffner thanks for your initial review.

The JOSS review process is slightly different from a traditional paper review in that you'll use this Gitlab issue and/or issues on the software repository to communicate with the authors as you work though the review. So far, you've checked boxes which have been sufficiently addressed, and you've posted here about missing features, which is exactly how the process starts. As the authors work to address the issues and as you confirm the issues have been addressed, you'll check off more and more checkboxes until all requirements have been met.

Once both reviewers on the paper have checked off all of the boxes in their respective checklists, I'll look over the paper and run through some finalization steps.

Hope that makes sense - feel free to ping me again if you have any questions or concerns.

vinisalazar commented 2 years ago

Hi @ggonnella,

I rebuilt the Docker image today after the updates. I still can successfully run the benchmarks from the Docker container, but I was having trouble with the genome-attributes-viewer app, it started, I ran a couple of queries, and then it crashed with this traceback:

root@d8798c592652:/fastsubtrees/docker# ./start-example-app
Dash is running on http://0.0.0.0:8050/

 * Serving Flask app 'start' (lazy loading)
 * Environment: production
   WARNING: This is a development server. Do not use it in a production deployment.
   Use a production WSGI server instead.
 * Debug mode: on
./start-example-app: line 15:   823 Killed                  ./start.py
root@d8798c592652:/fastsubtrees/docker# Traceback (most recent call last):
  File "/fastsubtrees/genomes-attributes-viewer/start.py", line 171, in <module>
    app.run_server(debug=True, host='0.0.0.0')
  File "/usr/local/lib/python3.10/dist-packages/dash/dash.py", line 2033, in run_server
    self.server.run(host=host, port=port, debug=debug, **flask_run_options)
  File "/usr/local/lib/python3.10/dist-packages/flask/app.py", line 920, in run
    run_simple(t.cast(str, host), port, self, **options)
  File "/usr/local/lib/python3.10/dist-packages/werkzeug/serving.py", line 1000, in run_simple
    _rwr(
  File "/usr/local/lib/python3.10/dist-packages/werkzeug/_reloader.py", line 418, in run_with_reloader
    ensure_echo_on()
  File "/usr/local/lib/python3.10/dist-packages/werkzeug/_reloader.py", line 398, in ensure_echo_on
    termios.tcsetattr(sys.stdin, termios.TCSANOW, attributes)
termios.error: (5, 'Input/output error')

I was also able to run some fastsubtrees-query commands, which got me thinking it would be nice to have other attributes of a taxonomy entry (e.g. Scientific Name, Lineage, etc) in the output. Maybe this could be achieved by using taxonkit along with fastsubtrees.

I maintain my impression that fastsubtrees has legitimate and useful applications, but I think users will struggle to adopt it given the current installation procedure and command interface. I think that more focus could be given on having users easily (i.e. automatically) index and build the NCBI Taxonomy Tree and run queries on it. The genome-attributes-viewer app should also be able of being deployed automatically with a single command after install. Ideally, upon installing fastsubtrees with conda, users should already have a prebuilt NCBI Taxonomy Tree and could start the Dash app right away.

I also struggled with the output of the fastsubtrees-attr-query command, for example:

root@d8798c592652:/fastsubtrees# fastsubtrees-attr-query ncbi-taxonomy.tree GC_content 1129
2022-10-11 03:51:44 SUCCESS: Tree loaded from file "ncbi-taxonomy.tree"
2022-10-11 03:51:44 INFO: Subtree of node 1129 has size 1762
[None, None, None, None, None, [0.49187], None, None, None, None, [0.633354, 0.63345], [0.524473], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.563698], None, None, None, None, [0.600861], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.493706], None, None, None, [0.485047], None, None, None, None, None, None, None, None, None, None, None, None, [0.491264], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.604614], None, None, None, [0.597736], None, None, None, None, [0.603592], None, [0.644399], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.64059], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.541645], None, None, [0.602373], None, None, None, None, [0.584503], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.492615], [0.493446], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.613678], None, [0.590853], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.606186], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.607883], None, None, None, None, None, None, None, None, [0.58221], None, [0.599123], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.534223], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.406211], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.574981], None, None, None, None, [0.646371], [0.67057], [0.645688], [0.636933], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.534347], None, None, [0.549756], [0.583811], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.603577], None, [0.613608], [0.59087], [0.588178], [0.592676], [0.539637], [0.607604], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.49424], None, [0.581548], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.639157], None, None, [0.538882], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.679814], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.625627], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.66265], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.631121], None, None, None, [0.618893], None, None, None, None, None, None, [0.554843, 0.554375], [0.554356], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, [0.55123], None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None]

This output is confusing to me. I understand that these are GC content values, but how can I see which value belongs to which node? When redirecting this to a file, for example, one gets this Python representation of a list, where it would be preferable to have a stdout of one value per line.

Best, Vini

ggonnella commented 2 years ago

Hi @vinisalazar,

ggonnella commented 2 years ago

@vinisalazar I addressed your concerns regarding the complexity of installation. Now all it takes to run the example application is to install it by a single command (pip install genomes_attributes_viewer) and run it by a single command (genomes-attributes-viewer) which automatically does the setup steps the first time it is called.

ggonnella commented 2 years ago

@vinisalazar I added functionality to add taxonomic names to the query output, without breaking the generality of the software. This is done by adding the names as an attribute to the tree. An example of creating the names and showing them in a query is given in README.

vinisalazar commented 2 years ago

Hi @ggonnella thank you for the updates.

  • query command: this can be a good idea for the NCBI taxonomy tree, but I would like to keep the software library generic, since I think it can be very useful for other trees as well, besides the NCBI taxonomy tree - at least I plan to use it for other applications as well in the future; thus I think this can be implemented as an additional wrapper script/application, which uses the library
  • the same I can tell for creating the tree for NCBI taxonomy; I can and will write a wrapper script which does what you suggest automatically, but I don't want to change the basic idea, that the library can be used for other trees as well

That is perfectly fine. I don't think it's a necessity to have this greater focus on the NCBI Taxonomy, since the underlying goal of the library is a different one. But, from a user's perspective, it would be nice to have easy access to that tree (i.e. having it be built automatically or even packaged along with the software).

I will wait for ggonnella/fastsubtrees#16, ggonnella/fastsubtrees#17 and ggonnella/fastsubtrees#19 to be finished and then I should be able to finish my review.

Best, V

ggonnella commented 2 years ago

@vinisalazar @KonradHoeffner The current version 2.0 addresses the concerns and comments raised until now.

In particular:

ggonnella commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

KonradHoeffner commented 2 years ago

I have checked all the boxes now as my acceptance blockers have all been addressed.

Minor points for improvement:

vinisalazar commented 2 years ago

Hi,

Version 2.0 addresses all of my major concerns, but I do have some (minor) outstanding points:

AttributeError: type object 'Tree' has no attribute 'construct_from_ncbi_dumps'



Presumably it should be `Tree.construct_from_ncbi_dump`, without the s at the end? It is important to double check that any code snippets on the README run without any problems.
ggonnella commented 2 years ago

@vinisalazar:

ggonnella commented 2 years ago

@KonradHoeffner:

ggonnella commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

vinisalazar commented 2 years ago

@kellyrowland I have now finished my review and recommend this paper to be accepted for publication.

Please let me know if there's anything else I should do.

Best, V

KonradHoeffner commented 2 years ago

A few comments from my final read:

KonradHoeffner commented 2 years ago

@editorialbot check repository

editorialbot commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.18 s (547.0 files/s, 40004.6 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Python                           64            486            778           3770
Markdown                         14            321              0           1092
make                              4             51             16            198
Bourne Shell                      5             22             13            134
TeX                               1             10              0            125
YAML                              4             13             20             67
Bourne Again Shell                4              0              2             58
Dockerfile                        1              4             14             29
SQL                               1              0              0              4
reStructuredText                  1              2              9              2
--------------------------------------------------------------------------------
SUM:                             99            909            852           5479
--------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 1813

ggonnella commented 2 years ago

A few comments from my final read:

  • example are provided -> examples

Fixed, thank you

  • no Python package currently allows: are there packages for other programming languages?

I removed "Python" from the sentence. No, I am not aware of any package, also for other programming languages.

  • with Apple M1 Pro CPU -> with an Apple M1 Pro CPU or with the Apple M1 Pro CPU (I'm not an English native speaker but it sounds better to me with an article)

I added an. Thank you.

  • better scales -> scales better

Changed. Thank you.

  • If possible, align the numbers in the tables so that each digit with the same significance has the same horizontal position (i.e. right aligned for integers or numbers with the same numbers of decimal places).

I don't think this is possible in Markdown.

  • write out the meaning of GC on the first usage

I did it now. Thank you.

Additionally I removed the self citation to the URL of fastsubtrees Git repository and of ntdownload subdirectory. I don't think it's necessary, since the paper PDF already contains a link. Instead I added to the text the pip installation command, the current version and the subdirectory for each of the mentioned sub-packages.

ggonnella commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

KonradHoeffner commented 2 years ago

It's a pity that the word count is limited for JOSS papers because the algorithm description and extended discussion of parallelism in https://github.com/ggonnella/fastsubtrees/issues/8 is really interesting and it would be really nice to have that in the paper itself.

ggonnella commented 2 years ago

(I found another typo and unified the formatting of fastsubtrees in the text to italics)

ggonnella commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

ggonnella commented 2 years ago

It's a pity that the word count is limited for JOSS papers because the algorithm description and extended discussion of parallelism in ggonnella/fastsubtrees#8 is really interesting and it would be really nice to have that in the paper itself.

I would also be glad if it would be possible to add it to the paper. Since both reviewer seemed to agree on this, I polished that text, made it slightly more compact and added to the paper in a separate branch. This would be the resulting paper:

~https://github.com/ggonnella/fastsubtrees/suites/8895052593/artifacts/406895074~

https://github.com/ggonnella/fastsubtrees/suites/8898907872/artifacts/407163672

( EDIT: replaced link with a version which is a couple of lines shorter. )

KonradHoeffner commented 2 years ago

I prefer the longer version. @kellyrowland is that acceptable even if it is over the word count?

ggonnella commented 2 years ago

@KonradHoeffner thanks for your initial review.

The JOSS review process is slightly different from a traditional paper review in that you'll use this Gitlab issue and/or issues on the software repository to communicate with the authors as you work though the review. So far, you've checked boxes which have been sufficiently addressed, and you've posted here about missing features, which is exactly how the process starts. As the authors work to address the issues and as you confirm the issues have been addressed, you'll check off more and more checkboxes until all requirements have been met.

Once both reviewers on the paper have checked off all of the boxes in their respective checklists, I'll look over the paper and run through some finalization steps.

Hope that makes sense - feel free to ping me again if you have any questions or concerns.

@kellyrowland As author I can't tell exactly, but from the comments of the two reviewers, see above, I imagine that all the boxes are now checked? How will this go on now? ... (It remains open the question about the additional paragraph, about parallelisation wrote after request from the reviewers, which could be gladly added, please see discussion above.)

KonradHoeffner commented 1 year ago

@kellyrowland: We are finished with the review, please proceed.