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]: Brightway: An open source framework for life cycle assessment #236

Closed
whedon opened this Issue Apr 13, 2017 · 18 comments

Comments

Projects
None yet
5 participants
@whedon
Copy link
Collaborator

whedon commented Apr 13, 2017

Submitting author: @cmutel (Chris Mutel)
Repository: https://bitbucket.org/cmutel/brightway2
Version: 2.1
Editor: @katyhuff
Reviewer: @amoeba
Archive: 10.5281/zenodo.556145

Status

status

Status badge code:

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

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) 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 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 (2.0.2)?
  • Authorship: Has the submitting author (@cmutel) made major contributions to the software?

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

  • 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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 13, 2017

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks for JOSS. @amoeba it looks like you're currently assigned as the reviewer for this paper 🎉.

⭐️ Important ⭐️

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As as reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all JOSS reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
@amoeba

This comment has been minimized.

Copy link
Collaborator

amoeba commented Apr 16, 2017

I've just made some time to sit down with this submission, though I am not yet done. Overall, this software is very clearly research software and I imagine this software is useful to researchers. The code is clean, includes documentation and helpful introductions so I commend the author on the hard work putting together that documentation. I've gone through a few of the easy checklist items and have put my notes below:

Repository: Is the source code for this software available at the repository url?

Sort of. @katyhuff please advise: The source code at the linked repository is a meta-package a set of individual packages. From an open source perspective, this is fine. But from a JOSS perspective, I wonder what we'll do about the archival ZIP for the submission. Thoughts?

License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?

Two issues:

  • LICENSE file named LICENSE.txt. It seems pedantic and unnecessary to ask the author to change the filename so I will ignore this difference unless the editor thinks otherwise.
  • LICENSE files do not contain the text of an OSI-approved software license. Suggest author choose a license and replaces the contents of these files with the text of the license.

Version: Does the release version given match the GitHub release (2.0.2)?

  • Code is not on GitHub but BitBucket. I think this is fine but the checklist item is vague as to whether this is a problem. I'll accept this difference and move on.
  • The BitBucket repository does contain a tag for the 2.0.2 release which is also the latest release of the software
  • The install instructions install version 2.1 of the software at present. This isn't an issue AFAICT from the JOSS guidelines but as a reviewer I'm wondering why there is a discrepancy here.

References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

No. Of the references used, none have DOIs and all but one reference (the developer's blog) should have DOIs. DOIs for the other software repositories this submission's repository contains relates to my above issue about this submission's code being split across numerous software repositories.

I will continue working on the rest of the checklist items now.

@cmutel

This comment has been minimized.

Copy link

cmutel commented Apr 16, 2017

Thanks @amoeba-

You are totally right that this is a "meta repository" - for maintainability and separation of concerns this decision was taken many years ago. No idea how this effects JOSS, but it is certainly the right choice for a large and evolving project.

@amoeba

This comment has been minimized.

Copy link
Collaborator

amoeba commented Apr 16, 2017

Totally agree with you on the meta-repo concept. Hopefully we can find a resolution that fits JOSS.

Re: Licensing, I'm not a lawyer but your License file says

Copyright (c) 2016, Chris Mutel and ETH Zurich
All rights reserved.

which doesn't seem to be compatible with the licensing requirements here. My understanding is that the license file would need to only include the BSD license text. Again, not a lawyer.

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Apr 16, 2017

@cmutel: 'All rights reserved' is the problematic line here. This isn't part of the standard BSD 3-clause license.

On the meta-repo concept: I think this is fine as long as the archive (with figshare or Zenodo) includes all of the associated repositories. Does that sound reasonable @amoeba and @cmutel?

@amoeba

This comment has been minimized.

Copy link
Collaborator

amoeba commented Apr 16, 2017

Yep, that sounds ideal.

@cmutel

This comment has been minimized.

Copy link

cmutel commented Apr 17, 2017

Sorry, that was an embarrassing mistake. And of course it was then copy/pasted in a million other places.

License is now fixed for all components, and new releases are on pypi and anaconda.

@amoeba

This comment has been minimized.

Copy link
Collaborator

amoeba commented Apr 17, 2017

I've sat down with the rest of the checklist and made good progress. Again, thanks @cmutel for the extensive documentation and example code. See my comments below:

  • 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).
  • 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

The author has a nicely put-together documentation page at https://docs.brightwaylca.org/ which does include this information. Can the content at that page be included in our archival ZIP? Usually the information fulfilling these three checklist items is contained inside the README(.md) on the source code repository which I think is the best place for such information due to the archive-oriented nature of JOSS. Given the meta-package aspect of this submission that we have already discussed, I understand this may not make as much sense.

I request that the author either update the README files to include this information or archive the content at https://docs.brightwaylca.org with the submission's archive.


  • Functionality: Have the functional claims of the software been confirmed?

I ran through some of the provided Jupyter notebooks and had no trouble running the code and seeing a reasonable result. After some reading (most the very helpful docs), I have only a vague idea what LCA is. I have no reason to believe the software is not working as intended but I can't say I'd be checking off this item with as much confidence as I have with other JOSS reviews I've done. Please advise @katyhuff ?


Otherwise, everything else is in order and I've checked off some remaining items. As it stands, I request the author review my comments on Installation/Example usage/Community guidelines and that the Version is rectified now that updates have been made since submission to JOSS.

Suggest: Accept with minor revisions and after discussion with @katyhuff about my concerns (above) over the Functionality checklist item (see above comments).

@cmutel

This comment has been minimized.

Copy link

cmutel commented Apr 17, 2017

@amoeba One small note on the documentation and usage examples - both the documentation and usage examples are part of the brightway2 "meta-package." Not sure how this is supposed to work - are the input (restructured text & jupyter notebook) files enough, or would the output (html) files also be needed?

@amoeba

This comment has been minimized.

Copy link
Collaborator

amoeba commented Apr 17, 2017

Thanks for linking those. I think what you've got there is sufficient. The JOSS guidelines are vague as to where such information exists and what form it exists in but I'd say what you've got counts:

From the guidelines:

A high-level overview of this documentation should be included in a README file (or equivalent).

@cmutel

This comment has been minimized.

Copy link

cmutel commented Apr 19, 2017

@katyhuff It seems like the outstanding issues have been addressed - would it be possible to complete this review this week? It would be very nice to be able to have a citation for a proposal due next Monday :)

@katyhuff

This comment has been minimized.

Copy link
Member

katyhuff commented Apr 19, 2017

@amoeba Thanks so much for this review. Regarding the funcitonality issue, this does look to be a useful framework for LCA and meets the functionality claims that are made.

@cmutel Thanks for addressing the comments quickly. At this point could you make an updated archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the version 2.1 archive? I can then move forward with accepting the submission!

@cmutel

This comment has been minimized.

Copy link

cmutel commented Apr 19, 2017

@katyhuff I uploaded the complete archive to Zenodo: https://zenodo.org/record/556145

DOI is 10.5281/zenodo.556145

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Apr 19, 2017

@whedon set 10.5281/zenodo.556145 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 19, 2017

OK. 10.5281/zenodo.556145 is the archive.

@arfon arfon added the accepted label Apr 19, 2017

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Apr 19, 2017

Thanks for your review @amoeba and for editing this one @katyhuff

@cmutel - your paper is now accepted into JOSS and your paper DOI is http://dx.doi.org/10.21105/joss.00236 ⚡️ 🚀 💥

@arfon arfon closed this Apr 19, 2017

@cmutel

This comment has been minimized.

Copy link

cmutel commented Apr 19, 2017

Many thanks to all involved!

@katyhuff

This comment has been minimized.

Copy link
Member

katyhuff commented Apr 19, 2017

Thanks @cmutel @amoeba and @arfon !

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