[REVIEW]: Phylogemetric: A Python library for calculating phylogenetic network metrics

[whedon]

Submitting author: @SimonGreenhill (Simon Greenhill) Repository: https://github.com/SimonGreenhill/phylogemetric Version: v1.0.0 Editor: @arfon Reviewer: @krother
Archive: http://dx.doi.org/10.5281/zenodo.55663

Status

Status badge code:

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

Reviewer questions

Conflict of interest

• [x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

  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] Version: Does the release version given match the GitHub release (v1.0.0)?
• [x] Archive: Does the software archive resolve?

  Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
• [x] Functionality: Have the functional claims of the software been confirmed?
• [x] Performance: Have any performance claims of the software been confirmed?

  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 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 function 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

Paper PDF: 10.21105.joss.00028.pdf

• [x] Authors: Does the paper.md file include a list of authors with their affiliations?
• [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] References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

[arfon]

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

• Please work through the checklist at the start of this issue.
• If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
• Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

[krother]

:hand: I am reviewing this

[krother]

Checked both doi-less references. They in fact have no doi.

[krother]

This is a small but very neat piece of software. It works smoothly and I understood everything (the Eigen paper on quartet analysis/statistical geometry was one of my favourites as a student). I have a couple of suggestions:

• One line on how to install the program (python setup.py install works for me) would be fine.
• One example (plus adding a small Nexus file to the repository) would make it a lot easier to try the program.
• Please use a standard license. There are good reasons not to write your own legal text (https://www.youtube.com/watch?v=H_X9TK_VJak)
• There is no community guideline. In my opinion this is not crucial because the project is small and has an awesome test suite (running tests could be mentioned).

From an engineering point of view, I find it very convincing. Good job!

[krother]

Another idea: since the Holland paper is available in full-text it could be hyperlinked from the README.

[krother]

I am currently running a simple performance benchmark on the software. I generated 100 sequences of length 100. Finished in 1:30 on my machine. Then I generated 1000 sequences of length 100. Still running after 30 minutes. It might be worth mentioning what size of input the software is intended for (or its O(n) behaviour). Just to give students a chance to see the warning sign before they push all known bacterial genomes through phylogemetric. No optimization required in my opinion.

Addendum: terminated run with 1000 sequences after 900 minutes.

[arfon]

• Please use a standard license. There are good reasons not to write your own legal text (https://www.youtube.com/watch?v=H_X9TK_VJak)

@krother, I'm pretty sure this is a standard BDS 3-Clause license: https://opensource.org/licenses/BSD-3-Clause

[danielskatz]

@arfon, it's close, but not quite standard.

[arfon]

@arfon, it's close, but not quite standard.

@danielskatz - the only difference I see here is that they've substituted 'copyright holder' for 'Treemaker' on this line:

1. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.

Am I missing something?

[danielskatz]

No, but that seems to me to be a potentially important difference, and one that doesn't have any benefit.

[arfon]

No, but that seems to me to be a potentially important difference, and one that doesn't have any benefit.

👍 ok. I agree that for clarity this should be switched back to the standard language. @SimonGreenhill - would you mind switching the language back to the license text here: https://opensource.org/licenses/BSD-3-Clause

[SimonGreenhill]

Hi all, thanks for the comments/suggestions, I'll make them ASAP.

I had intended a generic BSD-3 clause, so not sure what happened (perhaps I did a dumb search and replace that messed something up -- TreeMaker is another package I wrote). Will revert to BSD-3

[arfon]

• Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.

@danielskatz I've been investigating how GitHub handles the templating of the BSD 3-clause license. It looks like we swap out copyright holder for the name of the repository: see here for example.

As @SimonGreenhill mentioned, in this case it looks like TreeMaker is the name of another package but it seems like we should be tolerant/allow people to replace copyright holder with an actual name here.

/ cc @mlinksva in case he has a chance to give feedback here :grin:

[danielskatz]

@arfon I disagree with you about the templating. In clause 3, I'm fairly sure "copyright holder" is part of the license text, not a template variable to be filled in. See https://opensource.org/licenses/BSD-3-Clause This makes it clear (to me, at least) that the only variation in the license should be in the first line.

[arfon]

@danielskatz - sorry, should have clarified: I wasn't trying to say this was the definitely the 'correct' behaviour, rather just try and understand why/how 'copyright holder' has been swapped out for something else. It seems like this is probably GitHub's fault.

It does seems like this treating this as a template variable might be an error on GitHub's part here hence my ping of @mlinksva

[mlinksva]

There's not a single right answer, but I agree less templated fields is good. This has been recently fixed in https://github.com/github/choosealicense.com/pull/415 (and see discussion there for some of the history of the license) and has been vendored into https://github.com/benbalter/licensee/releases/tag/v8.2.0 which will make its way into the github.com license chooser when it gets that or a later version of licensee. This will result in fewer projects with license files with clause 3 templated, but there's nothing wrong with license files that do have that clause templated.

[danielskatz]

For JOSS, If there's a standard, and we get submissions that don't use the standard, we shouldn't accept them.

The license is still valid, and if that's what the creators want to use, that's fine, but in the context of JOSS, I don't think this is what we want.

[krother]

For me, the benefit of standard licenses is that I can consult Google instead of a lawyer whether two licenses are compatible. @danielskatz I second your point.

[arfon]

@danielskatz - agreed.

@SimonGreenhill - I think you offered already but just to confirm - please switch this out to the standard license text :smile_cat:

[SimonGreenhill]

@arfon - done. Sorry for the confusion! I'll push an update soon that fixes the license issue, and the review comments by @krother

[SimonGreenhill]

Ok, all changes suggested by @krother have been made, and the LICENSE file has been updated. Many thanks for all your suggestions.

[krother]

Great, I like the improvements! Just a tiny request that got under the wheels: could you label the License as BSD-3 somewhere, so that it is easy to find out which one it is? No other obstacles from my side.

[SimonGreenhill]

Sure, how's this?

[arfon]

Sure, how's this?

Looks good to me @SimonGreenhill.

Thanks for the rapid review here @krother. I've updated the checklist to make it clear that the license is now updated.

@SimonGreenhill - your DOI is http://dx.doi.org/10.21105/joss.00028 🎉 🚀 :boom:

[SimonGreenhill]

Thanks everyone!