search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: PyBox: An automated box-model generator for atmospheric chemistry and aerosol simulations. #755

Closed whedon closed 6 years ago

whedon commented 6 years ago

Submitting author: @loftytopping (David Topping) Repository: https://github.com/loftytopping/PyBox Version: v1.0.0 Editor: @lheagy Reviewer: @dvalters, @highendcompute Archive: 10.5281/zenodo.1345005

Status

status

Status badge code:

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

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

@dvalters & @highendcompute, 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 @lheagy know.

Review checklist for @dvalters

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @highendcompute

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

lheagy commented 6 years ago

:wave: Many thanks @dvalters and @highendcompute for being willing to review! In the main comment thread of this issue, there is a checklist for you both as you go through the review. Please don't hesitate to get in touch with any questions!

lheagy commented 6 years ago

@highendcompute informed me of a potential conflict of interest as he has published with @loftytopping in the past two years and has ongoing projects (that are not directly related to this one). As we have two reviewers, we will waive the conflict of interest and continue to have @highendcompute as a reviewer here. Please let me know if you have any concerns.

loftytopping commented 6 years ago

This sounds fine to me, many thanks! I will be using PyBox with students in our faculty for their final year projects. Some are starting over the summer and I have created some additional instructions on how to run PyBox in a Docker container. Im guessing I should now wait until the review has finished before uploading these to the repository? Absolutely fine, of course, if so.

dvalters commented 6 years ago

@loftytopping Normally in the past for JOSS I have just reviewed the linked tagged release, and the master or other branches can be continued to be worked on as normal

[edit] i.e. as long as you don't push to https://github.com/loftytopping/PyBox/tree/1.0.0 it should be fine

loftytopping commented 6 years ago

Ok thats good to know, thanks!

loftytopping commented 6 years ago

Hi @lheagy Im on holiday from next Sunday and then back in work, following a conference, 11th July. Im just going through open papers and editorial jobs Im involved in to just let everyone know so there was no unsuspecting silence from me. Thnx, Dave.

highendcompute commented 6 years ago

Hi all, should say I'm just between vacation and heading to ISC conference but aiming to use "hotel time" to initiate/complete the review. Apologies for delay. Yours, M

dvalters commented 6 years ago

Hi @loftytopping - sorry busy couple of weeks here too! I'm intending to make time to finish the review this week.

loftytopping commented 6 years ago

Hi guys. No problem, I appreciate it can get a little crazy post academic seasons. Its been the same for me. Though on recent travels Ive met a few people who are already using PyBox which has been nice.

loftytopping commented 6 years ago

Hi guys. Is it possible to have a projected update? I have a couple of groups starting to use it and approach study draft publications. Thanks!

dvalters commented 6 years ago

@loftytopping Will finish the review today :smile: (I promise this time)

loftytopping commented 6 years ago

No worries! Sorry for the prod, I guess its the double edged sword of releasing a package of use to a few groups!!

dvalters commented 6 years ago

Will resume the last bits of testing tonight - I've ticked off the checklist above where I believe it passes so far (mostly really good - don't see any major barriers to it being accepted).

Comments on Installation procedure

My only comment really at the minute is on the installation procedure. Outwith the docker image, it's fairly tricky to install. Just been trying to install it on a university Linux machine this morning and without docker/conda it's proved tricky - had issues getting Assimulo to work, linking to the right libs etc..

Having a setup.py file or as a minimum a requirements.txt file might help some of these issues? (For example, the dependencies section of the documentation lists third party modules as well as built-in modules (like shutil and multiprocessing), and also doesn't specify whether the third party modules have minimum versions.) A conda installation would be great although I realise that's probably quite a bit of extra work.

Edit: Just some further installation comments (could move this to an issue, although it's not really anything wrong per se.)

In addition, there are all the dependencies to UManSysProp, e.g. OpenBabel and the pybel API. Confusingly there is also a PyPI package called PyBEL, which will be installed if you type pip install pybel. I found I could install openbabel + pybel with:

conda install -c openbabel openbabel

which gives the right pybabel module, and the Openbabel libraries.

It also indirectly depends on cPickle (which is not available in Python 3, it's been renamed pickle), xlsxwriter, flask and flask-WTF packages. I opened an issue in UManSysProp rather than the PyBox repository as the API for flask extensions has changed if you install flask/flask-WTF with pip, e.g. pip install flask flask-WTF. (https://github.com/loftytopping/UManSysProp_public/issues/4)

(alternatively you could specify an older version of flask in a requirements.txt file.)

After all the requirements were satisfied, I could run it and run the test cases above successfully :smile:

dvalters commented 6 years ago

I could run it and run the test cases above successfully

With the exception of the one referenced above, i.e. https://github.com/loftytopping/PyBox/issues/4

loftytopping commented 6 years ago

Many thanks @dvalters for spending time on this. As for the general installation procedure, the problem is for sure with Assimulo. It is also platform dependenent. Im not sure I could provide a generic script to ensure this is installed automatically as, as you say, there is a reliance on library locations etc. I completely agree that this could be better discussed and, if its ok with you, I will do this now as a solution to any perceived problem.

dvalters commented 6 years ago

@loftytopping Sure that's fine. It's not a JOSS requirement/barrier to acceptance to not have an automated setup and as long as the instructions are detailed enough that is fine in my opinion.

highendcompute commented 6 years ago

Hi, been away. I’ll pick it up with a view to completing by fri 10 aug if that is okay? Yours, M

From: David Topping notifications@github.com Sent: 31 July 2018 08:00 To: openjournals/joss-reviews joss-reviews@noreply.github.com Cc: High End Compute enq@highendcompute.co.uk; Mention mention@noreply.github.com Subject: Re: [openjournals/joss-reviews] [REVIEW]: PyBox: An automated box-model generator for atmospheric chemistry and aerosol simulations. (#755)

Hi guys. Is it possible to have a projected update? I have a couple of groups starting to use it and approach study draft publications. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/755#issuecomment-409116514 , or mute the thread https://github.com/notifications/unsubscribe-auth/AMZ2shXdkmf5vKSsDajbT2AH1fvlCnulks5uMABygaJpZM4URsOj . https://github.com/notifications/beacon/AMZ2shCCnENeY4zWcXZyocxGS-noW7Efks5uMABygaJpZM4URsOj.gif

highendcompute commented 6 years ago

There are better options (generally I speak) than a bash script for platform independent installs based on dependencies, but it seems this isn’t a necessary requirement. The likes of

I have built using the Docker provided – which worked when I had sufficient space (so maybe need to give heads up on requirements?) and which was larger than expected but I don’t think it’s a requirement to provide the most stream line docker (eg how one does the layers matters; a trimmed down Linux to start with; and other ideas). I just need more time so can focus and complete my review. M

From: David Topping notifications@github.com Sent: 02 August 2018 10:07 To: openjournals/joss-reviews joss-reviews@noreply.github.com Cc: High End Compute enq@highendcompute.co.uk; Mention mention@noreply.github.com Subject: Re: [openjournals/joss-reviews] [REVIEW]: PyBox: An automated box-model generator for atmospheric chemistry and aerosol simulations. (#755)

Many thanks @dvalters https://github.com/dvalters for spending time on this. As for the general installation procedure, the problem is for sure with Assimulo. It is also platform depdenent. Im not sure I could provide a generic script to ensure this is installed automatically as, as you say, there is a reliance on library locations etc. I completely agree that this could be better discussed and, if its ok with you, I will do this now as a solution to any perceived problem.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/755#issuecomment-409859206 , or mute the thread https://github.com/notifications/unsubscribe-auth/AMZ2snChYbAZqNe4MyjA_Hh3Rv_5cdSdks5uMsE1gaJpZM4URsOj . https://github.com/notifications/beacon/AMZ2sh-owLDFI0bvIJx2yP9myLYvGLtSks5uMsE1gaJpZM4URsOj.gif

loftytopping commented 6 years ago

Sure, thanks! @dvalters ive now changed the README.md file to include more information on the dependencies according to Conda/Docker/other. Hopefully this adds more clarity to any installation procedure. @lheagy Im making changes to the current master branch, will any potential paper point to a new version specified by myself once everything has been approved? thanks!

dvalters commented 6 years ago

@loftytopping Just seen the latest fixes - thanks - will have a final lookover this evening, looks good to go otherwise

highendcompute commented 6 years ago

I pulled updates as of 3rd Aug, stopped and removed all previous pybox containers, removed all previous pybox, & ubuntu images (presume was intermediate later). From this point:

for the Dockerfile I had to add an extra "no" to prevent hanging on final question:

Do you wish the installer to prepend the Anaconda3 install location
to PATH in your /root/.bashrc ? [yes|no]
[no] >>> 
Appending source /root/anaconda3/bin/activate to /root/.bashrc
A backup will be made to: /root/.bashrc-anaconda3.bak

For this change to become active, you have to open a new terminal.

Thank you for installing Anaconda3!

===========================================================================

Anaconda is partnered with Microsoft! Microsoft VSCode is a streamlined
code editor with support for development operations like debugging, task
running and version control.

To install Visual Studio Code, you will need:
  - Administrator Privileges
  - Internet connectivity

Visual Studio Code License: https://code.visualstudio.com/license

Do you wish to proceed with the installation of Microsoft VSCode? [yes|no]
>>>

my general comments in terms of optimising a user's build via docker: 1) is it possible to use miniconda then build up only what is required? (base ubuntu vmem 83 MB to final layer being 4.4 GB) 2) It is recommended for Docker containers to NOT run as root, but as a user (set up in Dockerfile)

please note that I am happy with this current version meets the JOSS requirements, the above are merely suggestions/questions for potential improvements in future versions

I will open other comments re running the Docker image and on building outside of Docker

highendcompute commented 6 years ago

running from docker image, i appear to hit a problem:

[20:48:10] ~/DT_pybox/PyBox$ docker run -it pybox root@01a532291b93:/Code# whoami root root@01a532291b93:/Code# cd /Code/Git_repos/PyBox/ root@01a532291b93:/Code/Git_repos/PyBox# python Gas_simulation.py Opening file MCM_APINENE.eqn.txt for parsing Calculating total number of equations = 836 Parsing each equation Total number of species = 305 Saving all equation information to dictionaries Saving the mechanism dictionary as a pickled object for later retrieval Converting rate coefficient operations into Python-Numba file Creating Python-Numba functions and sparse matrices for product multiplications and dydt function Creating Python-Numba Jacobian Traceback (most recent call last): File "Gas_simulation.py", line 163, in <module> Parse_eqn_file.write_gas_jacobian_numba(filename,equations,num_species,loss_dict,gain_dict,species_dict2array,rate_dict_reactants) File "/Code/Git_repos/PyBox/Parse_eqn_file.py", line 2987, in write_gas_jacobian_numba f.write('#\xa7# \n') UnicodeEncodeError: 'ascii' codec can't encode character '\xa7' in position 1: ordinal not in range(128) root@01a532291b93:/Code/Git_repos/PyBox#

lheagy commented 6 years ago

Hi @loftytopping, to answer your question - any changes made to the paper before we accept it will be included in the published version

highendcompute commented 6 years ago

building on linux server (uname: Linux mkb-debian-vm 3.16.0-4-amd64 #1 SMP Debian 3.16.43-2+deb8u3 (2017-08-15) x86_64 GNU/Linux)

general comments (do not need fixing but please consider): ) maybe state if python2 is okay or whether it has to be python3 (I'm testing with python2) ) why both numpy and scipy? isn't it possible to use just one or the other

In terms of the dependencies for Python (not my primarily language I admit), Debian does not have any form of conda in its package manager suite nor do I see URL/.deb for alternative. It would be useful to clarify whether any Python package manager (eg pip) will suffice or whether there is a good reason to use anaconda.

In order to validate your assertions I have used a VM and run the anaconda install script. The following is based on this:

In terms of testing this build, I again get a (de)coding error:

[14:34:53] ~/DT_pybox/PyBox$ python Gas_simulation.py File "Gas_simulation.py", line 38 SyntaxError: Non-ASCII character '\xe2' in file Gas_simulation.py on line 38, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

dvalters commented 6 years ago

"conda install openbabel" however fails to find the required package.

@highendcompute I had to do conda install -c openbabel openbabel (i.e. it's not in the main conda repository)

loftytopping commented 6 years ago

@highendcompute thanks for the note re the Dockerfile. Ive now corrected this, it seems that an extra character prior to an EOL statement in the Numba file crept in when editing on my Mac. This has now been removed and tested in a new container. Ive now changed the README file and docs Index.md to include the full openbabel repository. More on the other items ASAP.

loftytopping commented 6 years ago

The addition of an extra "no" in the printf chain is not required and raises an error message on build completion. Unfortunately the MS Visual Studio component section is simply too slow, and I'm not sure why. It appears to hang, but does proceed if left. I will add a note on this in the Dockerfile readme attachment.

highendcompute commented 6 years ago

regarding the Dockerfile and my orginal comment about needing additional "no" in the printf chain. I have investigated and it appears this is not an issue for the Dockerfile but rather running Docker under a Linux VM with limited disk & memory attributes leads to extortionate long waits (typically at the point of removing intermediate containers). It is not necessary for this "paper" but future considerations may be to reduce the number of such and to use the 'batch' mode for Anaconda, eg RUN wget https://repo.continuum.io/archive/Anaconda3-5.1.0-Linux-x86_64.sh && bash -x Anaconda3-5.1.0-Linux-x86_64.sh -b && rm Anaconda3-5.1.0-Linux-x86_64.sh

highendcompute commented 6 years ago

i can now run the docker version. i will check the handbuild version tonight

loftytopping commented 6 years ago

Thanks @highendcompute . In response to your concerns, Ive added the following information,licensing and note on future builds to the Dockerfile. Based on Docker forums it seems this root issue is an ongoing debate:

All Rights Reserved.
This file is part of PyBox.
PyBox is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later version. PyBox is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with PyBox. If not, see http://www.gnu.org/licenses/. The default Docker container provides access as root. This might cause a security issue and it is important to check this with your local sysadmin where possible. The current image also builds to over 4GB. Please ensure you have adequate space to build and run. Please check the project Github wiki for planned updates to security and build procedures

loftytopping commented 6 years ago

Ive also now created an enhancement request on the project repo as : Lightweight Docker image #6 Ive assigned myself to sort this over the near future.

highendcompute commented 6 years ago

Hi, not sure why all these different comments/suggestions are in a single thread (@lheagy ?) but here's my penultimate comments:

On the parallel front, it is very good to see this included. I cannot test on my docker build since that is within a VM and the infrastructure does not support good parallel performance (tests done elswehere)

I shall now look at a CLI build of PyBox on native Linux system on multicore machine in order to feedback.

M

highendcompute commented 6 years ago

NB on some systems, going anaconda route I got some issues, as highlighted in the README.md as likely need install sundials explicitly, which I did via conda install -c chemreac sundials blas but still get

[15:48:51] ~/tmp/PyBox$ python Python 3.6.4 |Anaconda, Inc.| (default, Jan 16 2018, 18:10:19) [GCC 7.2.0] on linux Type "help", "copyright", "credits" or "license" for more information.

from assimulo.solvers import RodasODE, CVode Could not find not found (required by /mnt/iusers01/pp01/mccssmb2/anaconda3/lib/python3.6/site-packages/assimulo/lib/radau5.cpython-36m-x86_64-linux-gnu.so) Could not find not found (required by /mnt/iusers01/pp01/mccssmb2/anaconda3/lib/python3.6/site-packages/assimulo/solvers/sundials.cpython-36m-x86_64-linux-gnu.so) Could not find not found (required by /mnt/iusers01/pp01/mccssmb2/anaconda3/lib/python3.6/site-packages/assimulo/solvers/kinsol.cpython-36m-x86_64-linux-gnu.so) Could not find not found (required by /mnt/iusers01/pp01/mccssmb2/anaconda3/lib/python3.6/site-packages/assimulo/lib/dopri5.cpython-36m-x86_64-linux-gnu.so) Could not find not found (required by /mnt/iusers01/pp01/mccssmb2/anaconda3/lib/python3.6/site-packages/assimulo/lib/rodas.cpython-36m-x86_64-linux-gnu.so) Could not find not found (required by /mnt/iusers01/pp01/mccssmb2/anaconda3/lib/python3.6/site-packages/assimulo/lib/odassl.cpython-36m-x86_64-linux-gnu.so) Could not find ODEPACK functions Could not find RADAR5 Could not find GLIMDA Traceback (most recent call last): File "", line 1, in ImportError: cannot import name 'RodasODE'

(This has happened on two systems where there was a system anaconda but it wasn't possible to use for obtaining new packages (since tried to install in system dir) so used the Anaconda script directly to install in $HOME/anaconda3 and set PATH to include this as first element

lheagy commented 6 years ago

@highendcompute: These are all in one thread as you are replying on the JOSS review. I will transfer your comments to issues on the target repository, https://github.com/loftytopping/PyBox, so that @loftytopping can respond to them individually there.

lheagy commented 6 years ago

@loftytopping, @highendcompute: I have transferred your comments to issues

loftytopping commented 6 years ago

Thanks @lheagy ive now addressed those seperate issues.

highendcompute commented 6 years ago

with the resolution of all issues i am now happy with this to be published. do i need do anything else at this stage? m

lheagy commented 6 years ago

Excellent, thanks @highendcompute! There is nothing else you need to do at this stage. Thank you for your work on this review!

@dvalters: there is one remaining item on your checklist: functionality. Are there outstanding issues to be addressed here? Thanks!

dvalters commented 6 years ago

@lheagy Ticked it off - It is all good to go now!

lheagy commented 6 years ago

Thanks @dvalters!

@loftytopping can you please make an archive of the software on zenodo or similar and post the doi here?

loftytopping commented 6 years ago

Great. Here is the DOI URL: https://zenodo.org/badge/latestdoi/130043614 Many thanks to both the reviewers on this entire process, I appreciate the time and effort.

lheagy commented 6 years ago

@whedon set 10.5281/zenodo.1345005 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1345005 is the archive.

lheagy commented 6 years ago

👋 @arfon, this submission is ready to be published!

Thanks @dvalters and @highendcompute for your review! and congratulations @loftytopping 🎉

arfon commented 6 years ago

Thanks again @dvalters and @highendcompute for your review here and thanks to @lheagy for editing this submission ✨

@loftytopping - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00755 :zap: :rocket: :boom:

whedon commented 6 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

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

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

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.00755">
  <img src="http://joss.theoj.org/papers/10.21105/joss.00755/status.svg" alt="DOI badge" >
</a>

This is how it will look in your documentation:

DOI

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: