-
-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: Phylogemetric: A Python library for calculating phylogenetic network metrics #28
Comments
/ 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 Reviewer instructions
Any questions, please ask for help by commenting on this issue! 🚀 |
✋ I am reviewing this |
Checked both doi-less references. They in fact have no doi. |
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:
From an engineering point of view, I find it very convincing. Good job! |
Another idea: since the Holland paper is available in full-text it could be hyperlinked from the README. |
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. |
@krother, I'm pretty sure this is a standard BDS 3-Clause license: https://opensource.org/licenses/BSD-3-Clause |
@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:
Am I missing something? |
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 |
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 |
@danielskatz I've been investigating how GitHub handles the templating of the BSD 3-clause license. It looks like we swap out As @SimonGreenhill mentioned, in this case it looks like / cc @mlinksva in case he has a chance to give feedback here 😁 |
@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. |
@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 |
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. |
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. |
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. |
@danielskatz - agreed. @SimonGreenhill - I think you offered already but just to confirm - please switch this out to the standard license text 😸 |
Ok, all changes suggested by @krother have been made, and the LICENSE file has been updated. Many thanks for all your suggestions. |
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. |
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 🎉 🚀 💥 |
Thanks everyone! |
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:
Reviewer questions
Conflict of interest
General checks
Functionality
Documentation
Software paper
Paper PDF: 10.21105.joss.00028.pdf
paper.md
file include a list of authors with their affiliations?The text was updated successfully, but these errors were encountered: