[REVIEW]: Pymagicc: A Python wrapper for the simple climate model MAGICC

[whedon]

Submitting author: @rgieseke (Robert Gieseke)
Repository: https://github.com/openclimatedata/pymagicc
Version: v1.0.0
Editor: @arfon
Reviewer: @sgrieve
Archive: 10.5281/zenodo.1165153

Status

Status badge code:

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

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

@sgrieve, 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 (v1.0.0)?
• Authorship: Has the submitting author (@rgieseke) 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)?

Reviewer 2 instructions & questions

@scollis, 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 (v1.0.0)?
• Authorship: Has the submitting author (@rgieseke) 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. @sgrieve 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]

https://github.com/openjournals/joss-papers/blob/joss.00516/joss.00516/10.21105.joss.00516.pdf

[arfon]

@scollis - based on @rgieseke's comment (#502 (comment)) is this something you'd still be interested in being a second reviewer for?

[sgrieve]

I would like to thank the authors for this contribution, it has been a very enjoyable experience to explore this software and I am pleased to recommend publication, pending the minor issues highlighted below being resolved.

Installation

I tested the install from pip in both Python 3.6 and Python 2.7, both running on MacOS 10.12.6. I did not have wine installed, but the suggested homebrew install worked without problems. I had no problems with either install, and was able to quickly get the code running. I have also cloned the repo and got the code running locally, with tests passing for both Python 3.6 and Python 2.7.

Documentation

On first use in a clean Python 3.6 environment, with a fresh install of wine I got the following outputs when running the basic usage example in an Ipython terminal:

In [1]: import pymagicc
   ...: from pymagicc import scenarios
   ...: import matplotlib.pyplot as plt
   ...:
   ...: for name, scen in scenarios.items():
   ...:     results, params = pymagicc.run(scen, return_config=True)
   ...:     temp = (results["SURFACE_TEMP"].GLOBAL.loc[1850:] -
   ...:             results["SURFACE_TEMP"].GLOBAL.loc[1850:1900].mean())
   ...:     temp.plot(label=name)
   ...: plt.legend()
   ...: plt.title("Global Mean Temperature Projection")
   ...: plt.ylabel(u"°C over pre-industrial (1850-1900 mean)")
   ...:
wine: created the configuration directory '/Users/stuart/.wine'
err:ole:marshal_object couldn't get IPSFactory buffer for interface {00000131-0000-0000-c000-000000000046}
err:ole:marshal_object couldn't get IPSFactory buffer for interface {6d5140c1-7436-11ce-8034-00aa006009fa}
err:ole:StdMarshalImpl_MarshalInterface Failed to create ifstub, hres=0x80004002
err:ole:CoMarshalInterface Failed to marshal the interface {6d5140c1-7436-11ce-8034-00aa006009fa}, 80004002
err:ole:get_local_server_stream Failed: 80004002
err:ole:marshal_object couldn't get IPSFactory buffer for interface {00000131-0000-0000-c000-000000000046}
err:ole:marshal_object couldn't get IPSFactory buffer for interface {6d5140c1-7436-11ce-8034-00aa006009fa}
err:ole:StdMarshalImpl_MarshalInterface Failed to create ifstub, hres=0x80004002
err:ole:CoMarshalInterface Failed to marshal the interface {6d5140c1-7436-11ce-8034-00aa006009fa}, 80004002
err:ole:get_local_server_stream Failed: 80004002
fixme:ntdll:NtLockFile I/O completion on lock not implemented yet
err:mscoree:LoadLibraryShim error reading registry key for installroot
err:mscoree:LoadLibraryShim error reading registry key for installroot
err:mscoree:LoadLibraryShim error reading registry key for installroot
err:mscoree:LoadLibraryShim error reading registry key for installroot
fixme:ntdll:NtLockFile I/O completion on lock not implemented yet
err:winediag:SECUR32_initNTLMSP ntlm_auth was not found or is outdated. Make sure that ntlm_auth >= 3.0.25 is in your path. Usually, you can find it in the winbind package of your distribution.
fixme:dwmapi:DwmIsCompositionEnabled 0x6dbd1518
fixme:winsock:set_dont_fragment IP_DONTFRAGMENT for IPv4 not supported in this platform
fixme:iphlpapi:NotifyIpInterfaceChange (family 0, callback 0x69ebd3de, context 0x9c6560, init_notify 0, handle 0x12af190): stub
fixme:ntdll:NtLockFile I/O completion on lock not implemented yet
err:winediag:SECUR32_initNTLMSP ntlm_auth was not found or is outdated. Make sure that ntlm_auth >= 3.0.25 is in your path. Usually, you can find it in the winbind package of your distribution.
fixme:winsock:set_dont_fragment IP_DONTFRAGMENT for IPv4 not supported in this platform
fixme:iphlpapi:NotifyIpInterfaceChange (family 0, callback 0x6a0cb608, context 0x9387c0, init_notify 0, handle 0x116f454): stub
wine: configuration in '/Users/stuart/.wine' has been updated.

It might be an idea in the install instructions to mention that this might happen on the first run of the tool (this is mentioned in the notebook but not in the readme). To a less technical user this may cause concern that the code is not functioning correctly.

Running the basic usage example required me to run plt.show() to actually display the graph. It would be useful to provide a note in the code block to either use %matplotlib inline or plt.show() to display plots.

output = pymagicc.run(scenario)

# Projected temperature adjusted to pre-industrial mean
temp = output["SURFACE_TEMP"].GLOBAL - \
       output["SURFACE_TEMP"].loc[1850:2100].GLOBAL.mean()

Using the \ to break long lines is not recommended in PEP8, a better form for this would be to use parentheses to break the line:

output = pymagicc.run(scenario)

# Projected temperature adjusted to pre-industrial mean
temp = (output["SURFACE_TEMP"].GLOBAL
        - output["SURFACE_TEMP"].loc[1850:2100].GLOBAL.mean())

There are no instructions on how to run the tests within the readme. I was able to get them running easily enough using pytest, and they all passed, but it would be great to have this outlined in the readme.

One area that was lacking in the readme, was any information on how to contribute to the project, seek help or report issues. Can the authors add a simple statement to the end of the readme to cover this?

Tests

Looking at the coverage report, lines 271 and 322 are not hit by any of the tests. For completeness it would be nice to see these lines tested.

Paper

I would move 'Pymagicc utilizes the f90nml (Ward 2017) library' the citation to after the word 'library' in this sentence.

Check the formatting of the IPCC 2014 citation - it looks like 'Iii' should be either 'iii' or 'III'.

Is there no DOI for the IPCC report?

Notebooks

The title of the Demo notebook is 'Work-In-Progress ... Demo Notebook'. Is this correct?

The Usage Examples notebook is excellent, a really great example of a when it is useful to produce a notebook. I may use it in future classes as an example for my students! However, the demo notebook could use some markdown text in between the code cells to better describe exactly what is going on.

A minor niggle, in a few cells in the notebooks there are some long lines that cause horizontal scrolling in cells. If possible please shorten lines to around 80 characters, as long as it wont harm readability.

[rgieseke]

Thanks @sgrieve for the detailed and thorough review, this is much appreciated!

Find comments addressing your questions below:

I would like to thank the authors for this contribution, it has been a very enjoyable experience to explore this software and I am pleased to recommend publication, pending the minor issues highlighted below being resolved.

Thanks for the kind words!

Documentation
On first use in a clean Python 3.6 environment, with a fresh install of wine I got the following outputs when running the basic usage example in an Ipython terminal:

[...]

It might be an idea in the install instructions to mention that this might happen on the first run of the tool (this is mentioned in the notebook but not in the readme). To a less technical user this may cause concern that the code is not functioning correctly.

Good point. Added in openscm/pymagicc@7c03d31

Running the basic usage example required me to run plt.show() to actually display the graph. It would be useful to provide a note in the code block to either use %matplotlib inline or plt.show() to display plots.

Added in openscm/pymagicc@a2062ba - this should it make it easier for less experienced Python developers.

temp = output["SURFACE_TEMP"].GLOBAL -
output["SURFACE_TEMP"].loc[1850:2100].GLOBAL.mean()
Using the \ to break long lines is not recommended in PEP8, a better form for this would be to use parentheses to break the line:

Good catch, I made a similar change in the first example a while ago, but not here. Thanks!
openscm/pymagicc@e556052

There are no instructions on how to run the tests within the readme. I was able to get them running easily enough using pytest, and they all passed, but it would be great to have this outlined in the readme.

Good point, added in openscm/pymagicc@7835379

One area that was lacking in the readme, was any information on how to contribute to the project, seek help or report issues. Can the authors add a simple statement to the end of the readme to cover this?

Done: openscm/pymagicc@28a88fd

Tests
Looking at the coverage report, lines 271 and 322 are not hit by any of the tests. For completeness it would be nice to see these lines tested.

Thanks! I've added more tests (openscm/pymagicc@36e9fd8).

For the first line I reverted the test again because it depends on running the model in the same minute, which is difficult to ensure in CI (openscm/pymagicc@27d4b86)

As it would essentially be testing the file writing of Python I don't think it's important to have it.

Note that there seems to be a problem with codecov report display at the moment, running it locally with

pytest --cov --cov-report=html

shows coverage at 91%.

Paper
I would move 'Pymagicc utilizes the f90nml (Ward 2017) library' the citation to after the word 'library' in this sentence.
Check the formatting of the IPCC 2014 citation - it looks like 'Iii' should be either 'iii' or 'III'.
Thanks, fixed! openscm/pymagicc@b607b64

Is there no DOI for the IPCC report?

Surprisingly not, there seems to be one for the report of WG I but not here, it does have an ISBN code however.

Notebooks
The title of the Demo notebook is 'Work-In-Progress ... Demo Notebook'. Is this correct?

Yes, it is "Work-In-Progress" which I started recently, trying to build an interactive tool using the Notebook 'appmode' extension. I hope to get around to improving this soon.

The Usage Examples notebook is excellent, a really great example of a when it is useful to produce a notebook. I may use it in future classes as an example for my students! However, the demo notebook could use some markdown text in between the code cells to better describe exactly what is going on.

Thanks, that's great to hear. The Demo notebook will be updated soon.

A minor niggle, in a few cells in the notebooks there are some long lines that cause horizontal scrolling in cells. If possible please shorten lines to around 80 characters, as long as it wont harm readability.

Fixed. openscm/pymagicc@60bec3b

[sgrieve]

I'm happy with all these changes, delighted to accept @arfon 🚀

[arfon]

Friendly reminder to take a look at this when you get a chance @scollis

[scollis]

Thanks! will do today

[scollis]

Still having install issues.. I do not use Brew.. I can not use it on this mac (work mac)... I have installed the macos app

evs351996:~ scollis$ which wine
/Applications/Wine Stable.app/Contents/Resources/wine/bin/wine

@rgieseke is it out of scope this should work?

[rgieseke]

@scollis It should work, and I just tested on my machine with a package installed from https://dl.winehq.org/wine-builds/macosx/download.html

I've created an issue in the Pymagicc repo
openscm/pymagicc#16

[scollis]

Thanks! The main issue I think I am having is with Conda environments.. I'll build a sandboxed environment.. I am on the road at the moment and I think this will be best checked on my Linux laptop back home (key word: "Mine").. Will be done soon, sorry for the hold up

[rgieseke]

The demo notebook has been updated. It took me a couple of failed attempts at getting Bokeh to work with ipywidgets but now it's a simple demo (using standard Matplolib) showing what can be done with Pymagicc and Jupyter Notebooks and Appmode.

https://mybinder.org/v2/gh/openclimatedata/pymagicc/master?urlpath=apps/notebooks/Demo.ipynb

[scollis]

Awesome.. Friday.. I promise :)

[scollis]

Do you expect this to work under py35?
with the demo notebook I get

TraitError: The 'options' trait of a Dropdown instance must be a list or a dict, but a value of array(['FossilCO2', 'OtherCO2', 'CH4', 'N2O', 'SOx', 'CO', 'NMVOC', 'NOx',
       'BC', 'OC', 'NH3', 'CF4', 'C2F6', 'C6F14', 'HFC23', 'HFC32',
       'HFC43-10', 'HFC125', 'HFC134a', 'HFC143a', 'HFC227ea', 'HFC245fa',
       'SF6'], dtype=object) <class 'numpy.ndarray'> was specified.

[scollis]

Okay! Success!
On my Ubuntu machine I was able to install via Pip and run the Example Script.

RECOMMENDATION: Suggest you name the notebooks better and guide the new user to run "Example" first. This notebook is much more informative on how PyMagicc is run.

[scollis]

@arfon Is Creative Commons an OSI Approved Licence?

[scollis]

@sgrieve Please update the repo's README.md with some verbage with the paper that describes the need.. eg something along the lines of "Aiming at broadening
the user base of MAGICC 1 , Pymagicc provides a wrapper around the MAGICC binary 2 ,
which runs on Windows and has been published under a Creative Commons Attribution-
NonCommercial-ShareAlike 3.0 Unported License 3:

[scollis]

Ok! My review is done! This is nice work. I can see myself using this software.

I have two concerns:

1. Is the Licence OSI approved?
2. I would like to see one more sentence in README.md to satisfy the JOSS review criteria that states you need a statement of need and clearly state the target audience.

Once these are done I will check the boxes and we are done!

[swillner]

Great!
We now improved the README.md as well as the paper.md. We made clearer the usage and target audience and included information on the license. While the MAGICC binary itself is CC-licensed, which is not OSI approved (https://opensource.org/licenses/alphabetical), Pymagicc is AGPL-3.0-licensed, which is.
Thanks for the review!

[rgieseke]

The diff is here: openscm/pymagicc@3338e25

As for the license, we only mention the CC-license as we distribute the MAGICC binary as part of the package and users need to be aware of the non-commercial clause. Pymagicc itself has no such restrictions. As there are no code linkages, we are only "wrapping" and running the binary there is no license inheritance.

We hope that the distinction of the two separately licensed parts is now clearer, thanks for pointing that out!

[rgieseke]

There is now also a Readme in the notebooks sub-directory to clarify how to use and run these Notebooks.

https://github.com/openclimatedata/pymagicc/tree/master/notebooks

[arfon]

OK, looks like we're ready to proceed here. @rgieseke - 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.

[rgieseke]

Thanks @arfon - I've tagged a v1.0.2 release with the edits and improvements from the reviews. It's available at

https://zenodo.org/record/1165153

https://doi.org/10.5281/zenodo.1165153

[arfon]

@whedon set 10.5281/zenodo.1165153 as archive

[whedon]

OK. 10.5281/zenodo.1165153 is the archive.

[arfon]

@sgrieve @scollis - many thanks for your reviews here ✨

@rgieseke - you paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00516 ⚡️ 🚀 💥

[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.00516/status.svg)](https://doi.org/10.21105/joss.00516)

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