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]: sourmash: a library for MinHash sketching of DNA #27

Closed
whedon opened this Issue Jun 12, 2016 · 17 comments

Comments

Projects
None yet
4 participants
@whedon
Collaborator

whedon commented Jun 12, 2016

Submitting author: @ctb (C. Titus Brown)
Repository: https://github.com/dib-lab/sourmash/
Version: 1.0
Editor: @arfon
Reviewer: @jkahn
Archive: 10.5281/zenodo.153989

Status

status

Status badge code:

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

Reviewer questions

Conflict of interest

  • 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

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (0.9.2)?
  • Archive: Does the software archive resolve?

Functionality

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

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the functionality of the software documented to a satisfactory level (e.g. API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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.00027.pdf

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

@whedon whedon added the review label Jun 12, 2016

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Jun 12, 2016

Member

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

Member

arfon commented Jun 12, 2016

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

@jkahn

This comment has been minimized.

Show comment
Hide comment
@jkahn

jkahn Jun 13, 2016

I am reviewing this submission. I should have a review by end of week; likely sooner.

jkahn commented Jun 13, 2016

I am reviewing this submission. I should have a review by end of week; likely sooner.

@jkahn jkahn self-assigned this Jun 15, 2016

@ctb

This comment has been minimized.

Show comment
Hide comment
@ctb

ctb Jun 15, 2016

Collaborator

Hi @jkahn thx - note that I just updated master with a bug fix (and a test for the bug :) for a bug discovered yesterday.

Collaborator

ctb commented Jun 15, 2016

Hi @jkahn thx - note that I just updated master with a bug fix (and a test for the bug :) for a bug discovered yesterday.

@jkahn

This comment has been minimized.

Show comment
Hide comment
@jkahn

jkahn Jun 16, 2016

I am not a copyright lawyer, but I don't think the text given in the release is exactly what the BSD 3-clause would say. For one thing, the Regents of the U of C and the MSU are mentioned the LICENSE file, but only the RofUofC in the header. (I understand that @ctb has probably moved from one to the other, looking at @dib-lab .

jkahn commented Jun 16, 2016

I am not a copyright lawyer, but I don't think the text given in the release is exactly what the BSD 3-clause would say. For one thing, the Regents of the U of C and the MSU are mentioned the LICENSE file, but only the RofUofC in the header. (I understand that @ctb has probably moved from one to the other, looking at @dib-lab .

@jkahn

This comment has been minimized.

Show comment
Hide comment
@jkahn

jkahn Jun 17, 2016

This is a nifty piece of software, combining a snappy algorithm with a pleasant Python API, and a model for good packaging and programmer/developer-friendly documentation (including working demo code, executable documentation, and a command-line tool!).

In the process of reviewing, I've opened a number of issues on the sourmash package itself, but I've only linked to this ticket on those issues that I feel should be resolved before a JOSS acceptance.

I recommend acceptance (after minor revisions).

As an afterword (and a comment to the Editors): Minting a new Zenodo snapshot & DOI before reviewing seems like wasted effort, since (I would hope) nearly all reviewers are going to request changes (even if minor) that will require a new Zenodo snapshot.
Can reviewers bless a particular tag or revision in the target repo, and have authors or JOSS take the zenodo snapshot/doi after the review process is complete?

jkahn commented Jun 17, 2016

This is a nifty piece of software, combining a snappy algorithm with a pleasant Python API, and a model for good packaging and programmer/developer-friendly documentation (including working demo code, executable documentation, and a command-line tool!).

In the process of reviewing, I've opened a number of issues on the sourmash package itself, but I've only linked to this ticket on those issues that I feel should be resolved before a JOSS acceptance.

I recommend acceptance (after minor revisions).

As an afterword (and a comment to the Editors): Minting a new Zenodo snapshot & DOI before reviewing seems like wasted effort, since (I would hope) nearly all reviewers are going to request changes (even if minor) that will require a new Zenodo snapshot.
Can reviewers bless a particular tag or revision in the target repo, and have authors or JOSS take the zenodo snapshot/doi after the review process is complete?

@jkahn

This comment has been minimized.

Show comment
Hide comment
@jkahn

jkahn Jun 17, 2016

updated the checklist so that there are unchecked boxes corresponding to the open questions.

jkahn commented Jun 17, 2016

updated the checklist so that there are unchecked boxes corresponding to the open questions.

@ctb

This comment has been minimized.

Show comment
Hide comment
@ctb

ctb Jun 17, 2016

Collaborator
Collaborator

ctb commented Jun 17, 2016

@ctb

This comment has been minimized.

Show comment
Hide comment
@ctb

ctb Jul 12, 2016

Collaborator

Hi @jkahn Thanks for all your comments - many excellent improvements resulted. I think this is ready for review once again!

Collaborator

ctb commented Jul 12, 2016

Hi @jkahn Thanks for all your comments - many excellent improvements resulted. I think this is ready for review once again!

@ctb

This comment has been minimized.

Show comment
Hide comment
@ctb

ctb Jul 12, 2016

Collaborator

Note, the latest version is 0.9.4, https://github.com/dib-lab/sourmash/releases/tag/v0.9.4, and zenodo etc should be updated soon

Collaborator

ctb commented Jul 12, 2016

Note, the latest version is 0.9.4, https://github.com/dib-lab/sourmash/releases/tag/v0.9.4, and zenodo etc should be updated soon

@jkahn

This comment has been minimized.

Show comment
Hide comment
@jkahn

jkahn Jul 18, 2016

Pending these two concerns (both resolved by a little bit more attention to onboarding new developers), I'm satisfied that my concerns have been addressed and recommend the package re-mint and be accepted.

I want to address my one continuing gripe: I don't love that the test code is mixed in (closed, wontfix) among the shipped code (in a public-facing library, this could open an entirely new attack surface) but I don't think it's a release-blocking offense, especially in a research package.

jkahn commented Jul 18, 2016

Pending these two concerns (both resolved by a little bit more attention to onboarding new developers), I'm satisfied that my concerns have been addressed and recommend the package re-mint and be accepted.

I want to address my one continuing gripe: I don't love that the test code is mixed in (closed, wontfix) among the shipped code (in a public-facing library, this could open an entirely new attack surface) but I don't think it's a release-blocking offense, especially in a research package.

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Jul 22, 2016

Member

Pending these two concerns (both resolved by a little bit more attention to onboarding new developers), I'm satisfied that my concerns have been addressed and recommend the package re-mint and be accepted.

👍 thanks @jkahn. @ctb - please let me know when you've made these changes.

Member

arfon commented Jul 22, 2016

Pending these two concerns (both resolved by a little bit more attention to onboarding new developers), I'm satisfied that my concerns have been addressed and recommend the package re-mint and be accepted.

👍 thanks @jkahn. @ctb - please let me know when you've made these changes.

@ctb

This comment has been minimized.

Show comment
Hide comment
@ctb

ctb Sep 13, 2016

Collaborator

OK, I've made the changes and @luizirber has checked them twice :).

Am I correct in thinking that I should proceed by cutting a new release (v1.0) and then putting the zenodo link in here? That's it, yes? (Reading author guidelines)

Collaborator

ctb commented Sep 13, 2016

OK, I've made the changes and @luizirber has checked them twice :).

Am I correct in thinking that I should proceed by cutting a new release (v1.0) and then putting the zenodo link in here? That's it, yes? (Reading author guidelines)

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Sep 13, 2016

Member

Am I correct in thinking that I should proceed by cutting a new release (v1.0) and then putting the zenodo link in here? That's it, yes? (Reading author guidelines)

👍 yes please @ctb

Member

arfon commented Sep 13, 2016

Am I correct in thinking that I should proceed by cutting a new release (v1.0) and then putting the zenodo link in here? That's it, yes? (Reading author guidelines)

👍 yes please @ctb

@jkahn

This comment has been minimized.

Show comment
Hide comment
@jkahn

jkahn Sep 13, 2016

sgtm! congratulations!

jkahn commented Sep 13, 2016

sgtm! congratulations!

@ctb

This comment has been minimized.

Show comment
Hide comment
@ctb

ctb Sep 14, 2016

Collaborator

sourmash 1.0 now up! DOI

Collaborator

ctb commented Sep 14, 2016

sourmash 1.0 now up! DOI

@ctb

This comment has been minimized.

Show comment
Hide comment
@ctb

ctb Sep 14, 2016

Collaborator

thanks for all your hard work @jkahn!

Collaborator

ctb commented Sep 14, 2016

thanks for all your hard work @jkahn!

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Sep 14, 2016

Member

Many thanks again for the review @jkahn!

@ctb your paper is now accepted in JOSS. Your DOI is http://dx.doi.org/10.21105/joss.00027 🎉 🚀 💥

Member

arfon commented Sep 14, 2016

Many thanks again for the review @jkahn!

@ctb your paper is now accepted in JOSS. Your DOI is http://dx.doi.org/10.21105/joss.00027 🎉 🚀 💥

@arfon arfon closed this Sep 14, 2016

@arfon arfon unassigned jkahn Aug 19, 2018

@arfon arfon changed the title from Submission: sourmash: a library for MinHash sketching of DNA to [REVIEW]: sourmash: a library for MinHash sketching of DNA Aug 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment