[REVIEW]: Temporalis: an open source software for dynamic LCA

[whedon]

Submitting author: @cardosan (Giuseppe Cardellini)
Repository: https://bitbucket.org/cardosan/brightway2-temporalis
Version: v1.0.1
Editor: @arfon
Reviewer: @pjamesjoyce
Archive: 10.5281/zenodo.1219211

Status

Status badge code:

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

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 instructions & questions

@pjamesjoyce, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

1. Make sure you're logged in to your GitHub account
2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arfon know.

Conflict of interest

• As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• I confirm that I read and will adhere to the JOSS code of conduct.

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 (1.0)?
• Authorship: Has the submitting author (@cardosan) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• Installation: Does installation proceed as outlined in the documentation?
• Functionality: Have the functional claims of the software been confirmed?
• Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

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]

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @pjamesjoyce 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 a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

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

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

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

@whedon commands

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

PDF failed to compile for issue #612 with the following error:

/app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in block in find': No such file or directory (Errno::ENOENT) from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in collect!'
from /app/vendor/ruby-2.3.4/lib/ruby/2.3.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-a31a5c2a9125/lib/whedon/processor.rb:52:in find_paper_paths'
from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-a31a5c2a9125/bin/whedon:32:in prepare' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/command.rb:27:in run'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor.rb:387:in dispatch'
from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-a31a5c2a9125/bin/whedon:99:in <top (required)>'
from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in load' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in

'

[arfon]

@pjamesjoyce - here's the compiled paper: 10.21105.joss.00612.pdf

@cardosan: when compiling this paper I got the following error, could you take a look at fixing it up?:

pandoc-citeproc: reference Beloin-Saint-Pierre2016 not found

[cardosan]

Hi @arfon, it should be fixed now.

[pjamesjoyce]

Looking good @cardosan.

I've added the following issues to the bitbucket repo:

#2: Statement of need, installation instructions and example usage missing from README
#3: Module takes a long time to import due to calculation of constants
#4: "ValueError: Can't rescale activities that produce zero reference product" for models where the reference product is different to the activity name
#5: BW2 models containing TemporalDistribution objects can't be exported as BW2Package files
#6: Suggested workaround in the Gotchas section for net zero emissions doesn't work
#7: If net emissions from upstream processes cancel each other out, the graph traversal skips the Activity
#8: Examples could be clearer and easier to find
#9: Example 2 contains a random process so results are not comparable

[cardosan]

Thanks @pjamesjoyce, seen them (some extremely helpful!!) .
Already started with the easier, will address the others in the coming days.

[pjamesjoyce]

Thanks for the fixes so far @cardosan. I've checked them out and they've resolved the issues successfully. I've closed issues #2, #3, #5, #6 and #7.

The fix for issue #3 led to a problem elsewhere for me (on windows). So I've added a couple more issues on bitbucket (one is more of a suggestion than an issue).

#10: Fix for Issue #3 creates bug in create_climate_methods() on Windows
#11: create_climate_methods() calls _create_constants() every time it runs

[pjamesjoyce]

Hi @cardosan. Thanks for fixing #10 and #11.

Once #8 and #9 are fixed I can sign off on the example usage element of the review.

For functionality #4 still needs fixing, and the automated tests don't currently pass, which I gather is due to the rounding issue you've identified (#1: test fails due to rounding).

There's also no test coverage (and only technical documentation, no examples) on the MultiDynamicLCA class. It works, although getting the right format for the methods was a bit tricky to figure out. Is this class meant to be part of the submission (in which case expanded documentation would be good), or is it an experimental extension?

[cardosan]

Hi @pjamesjoyce,
I have been quite busy lately writing the phd and getting ready for the delivery of my first child.....started to work on the example and the issue 4 but did not have time to finalize the work yet, will do in the coming days.

Regarding the test it is fixed now, while MultiDynamicLCA is still an experimental feature I would like to keep outside the review and the package before it is improved and better tested.

[cardosan]

Hi @pjamesjoyce,
I have addressed the issue you mentioned in the previous comment.

[arfon]

Thanks for the update @cardosan. @pjamesjoyce - please take another look when you get a chance.

[pjamesjoyce]

Looking good @cardosan.

I understand your explanation for issue #4 and for the purposes of the review, adding it as a known issue is ok. This is research software, and so researchers using it for temporal LCA should follow the modelling conventions required to use it. Retrofitting temporal distributions into existing models using different modelling conventions is a 'nice-to-have' (which can hopefully be added in the future when the bw2 database schema are updated). I've changed the status of the issue on bitbucket to 'on hold' until the new bw2 database schema come out. See my note on minor edits to the docs on bitbucket though.

Issue #9 is fixed.

I hit a couple of snags running the new example notebook for Issue #8, but all are minor and easily fixed. I've listed them on bitbucket with the workarounds I used to get it running properly.

Once the fixes to the example notebook are made I can close Issue #8 and approve your submission.

[pjamesjoyce]

Ok - the issues have all been resolved. @arfon, I approve this submission.

Great work @cardosan, a really nice piece of software and a great addition to the open source LCA toolkit.

[arfon]

Ok - the issues have all been resolved. @arfon, I approve this submission.

Great. Thanks @pjamesjoyce!

@cardosan - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

[cardosan]

Many thanks @pjamesjoyce for you valuable comments during the review!
@arfon - uploaded on zenodo https://zenodo.org/record/1219212.

DOI is 10.5281/zenodo.1219211.

Be aware that I submitted to JOSS version: 1.0 but during the review had to upgrade (and released) to 1.0.1 since the documentation on readthedocs was not correctly updating. Let me know if I need to change something in the submission.

[arfon]

@whedon set 10.5281/zenodo.1219211 as archive

[whedon]

OK. 10.5281/zenodo.1219211 is the archive.

[arfon]

@pjamesjoyce - many thanks for your review here ✨

@cardosan - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00612 ⚡️:rocket: :boom:

[whedon]

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippet:

[![DOI](http://joss.theoj.org/papers/10.21105/joss.00612/status.svg)](https://doi.org/10.21105/joss.00612)

This is how it will look in your documentation:

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

• Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
• Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=

[cardosan]

Thanks to all involved!