[REVIEW]: Phylogemetric: A Python library for calculating phylogenetic network metrics about joss-reviews HOT 25 CLOSED

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

from joss-reviews.

There's not a single right answer, but I agree less templated fields is good. This has been recently fixed in github/choosealicense.com#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.

from joss-reviews.

/ 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! 🚀

from joss-reviews.

✋ I am reviewing this

from joss-reviews.

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

from joss-reviews.

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!

from joss-reviews.

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

from joss-reviews.

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.

from joss-reviews.

• 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

from joss-reviews.

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

from joss-reviews.

@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?

from joss-reviews.

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

from joss-reviews.

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

from joss-reviews.

• 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 😁

from joss-reviews.

@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.

from joss-reviews.

@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

from joss-reviews.

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.

from joss-reviews.

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.

from joss-reviews.

@danielskatz - agreed.

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

from joss-reviews.

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

from joss-reviews.

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

from joss-reviews.

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.

from joss-reviews.

Sure, how's this?

from joss-reviews.

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 🎉 🚀 💥

from joss-reviews.

Thanks everyone!

from joss-reviews.