[REVIEW]: PyGBe: Python, GPUs and Boundary elements for biomolecular electrostatics about joss-reviews HOT 38 CLOSED

Version bump to 2.1 with all revisions.

10.5281/zenodo.60474

🎉🎉🎉

from joss-reviews.

✋ I'm happy to review this!

from joss-reviews.

Thanks, @kyleniemeyer ! We'll get on fixes for the pain points.
PyCUDA didn't support Cuda 7.5 last time we looked at upgrading, but it does now so we'll look into upgrading things.

from joss-reviews.

@kyleniemeyer Regarding comment number 2: (missing code DOI) ... we had a discussion on another thread concluding that submitting the code to Zenodo to get a DOI, and then doing it again after acceptance, resulted in wasted effort. (I looked for about 10 minutes but could not find the issue where this was discussed ... we have too many closed issues!).

Thus, if and when the software paper is accepted, the last step is to submit the code archive to Zenodo (or other repository assigning a DOI), and communicate the DOI to JOSS so it can be added to the paper. This is the final ceremony of publication in JOSS.

from joss-reviews.

I did run into an issue with the performance test: I was running this on a headless server, and when the test finished I got an error that seems to be the code attempting to display something

Ahh, yeah, X-dependent backend by default. This should be fixed on master now but I'm running it on a headless server to confirm.

Lastly, it looks like the documentation site (http://barbagroup.github.io/pygbe/) is now down.

Yikes. Not sure what happened there, but it's back up now. Thanks!

from joss-reviews.

Thank you also to reviewers # 2 and # 3: @keith923 and @lizutah, and marshalling by @sobolevnrm.

from joss-reviews.

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

from joss-reviews.

@kyleniemeyer - the paper has just been updated for this submission.

from joss-reviews.

Hi all—sorry, I've been traveling and at a conference for the last week (hello from Seoul!), and have been delayed on finishing this review. I'm going to make an effort to get this done in the next day or two...

from joss-reviews.

Alright, so I checked off the various items that are fine, but I have a few comments—and ran into some problems when trying to run the regression and performance tests.

1. The README is quite extensive and helpful (especially the instructions on building dependencies), but none of this information (installation, usage, examples, etc.) is actually contained in the documentation (http://barbagroup.github.io/pygbe/). I would recommend moving/repeating this information there, where it can be organized a bit more easily.
2. The paper is missing an archive/DOI of the version submitted (0.2), from (e.g.) Zenodo or figshare.
3. There are regression tests, but no unit tests (from what I can tell). This somewhat conflicts with the guidelines given in CONTRIBUTING.md, which says any new features need unit tests.
4. The support for CUDA versions below 7.0 seems somewhat restrictive, since we're on 7.5 now. I wasn't able to run the GPU-enabled version on my systems because of this. Would it be particularly challenging to support CUDA/nvcc 7.x?

I also have some comments on the paper:

1. There are some bugs with the affiliations for authors 1–3.
2. The summary could be a bit more general—as written, it isn't clear who would use the software and for what broad purpose.
3. The third reference is missing a DOI
4. You might consider adding references for the papers that have already used PyGBe (given in the README).

Regarding the tests, as I mentioned I couldn't run the GPU-enabled version of PyGBe, but it seems that I should have been able to run it in CPU-only mode. However, I ran into some errors with both the performance and regression tests, following the instructions. Both downloaded the external mesh files just fine, and seemed to go through some work (although I got frequent error messages and traceback for pytools.prefork.ExecError: error invoking 'nvcc --version': [Errno 2] No such file or directory).

The performance tests did not complete, with the error

Traceback (most recent call last):
  File "run_and_plot_lysozome_case.py", line 249, in <module>
    main()
  File "run_and_plot_lysozome_case.py", line 245, in main
    generate_plot(compiled_results, filetype='pdf')
  File "run_and_plot_lysozome_case.py", line 85, in generate_plot
    pyg_ext = richardson_extrapolation(res)
  File "run_and_plot_lysozome_case.py", line 60, in richardson_extrapolation
    esolv = compiled_results['E_solv_kJ']
KeyError: 'E_solv_kJ'

and the regression tests also led to an error:

Traceback (most recent call last):
  File "run_all_regression_tests.py", line 26, in <module>
    except FileNotFoundError:
NameError: name 'FileNotFoundError' is not defined

Finally, some minor comments:

Minor comments:

• It might be nice to give a recipe to create a conda environment, for those of us that mainly use Python 3 😃
• Running the regression (and I assume performance) tests requires matplotlib, which wasn't mentioned as a dependency
• I noticed that the paper author list doesn't completely match the authors listed in the LICENSE, which may or may not be important.

from joss-reviews.

Thanks, @kyleniemeyer for all the comments, we'll work on that.

About the errors that you're getting when you try to run the tests on CPU:
Did you notice that you need to change all the .param files on the input files folder to set GPU=0 (which force PyGBe to run on CPU)?
This is explained at the end of the README_input_format.

However, I would strongly recommend to run the regression tests on the GPU because they take a while, running them on the CPU version will take even longer.

from joss-reviews.

I apologize for the delayed reply; I've been swamped with reviews and editorial duties. I've asked @keith923 and @lizutah for help with the review.

from joss-reviews.

The README is quite extensive and helpful (especially the instructions on building dependencies), but none of this information (installation, usage, examples, etc.) is actually contained in the documentation (http://barbagroup.github.io/pygbe/). I would recommend moving/repeating this information there, where it can be organized a bit more easily.

The README is now duplicated in the github-pages documentation, along with the contribution guide and the input files reference.

There are regression tests, but no unit tests (from what I can tell). This somewhat conflicts with the guidelines given in CONTRIBUTING.md, which says any new features need unit tests.

Yeah, this is a 'do as I say, not as I do' thing. Unit tests are in the pipeline but since they're much harder to add to existing code than to new code, we're trying to foster good habits moving forward (for ourselves and others).

The support for CUDA versions below 7.0 seems somewhat restrictive, since we're on 7.5 now. I wasn't able to run the GPU-enabled version on my systems because of this. Would it be particularly challenging to support CUDA/nvcc 7.x?

Thanks for the kick in the rear -- PyGBe now works with Cuda <= 7.5

I also have some comments on the paper:

There are some bugs with the affiliations for authors 1–3.

I believe we have fixed this now.

The summary could be a bit more general—as written, it isn't clear who would use the software and for what broad purpose.

We have tried to address this with a few application examples.

The third reference is missing a DOI

Added. Thanks.

@arfon, could we prevail upon you to regenerate the PDF?

Regarding the tests, as I mentioned I couldn't run the GPU-enabled version of PyGBe, but it seems that I should have been able to run it in CPU-only mode. However, I ran into some errors with both the performance and regression tests, following the instructions. Both downloaded the external mesh files just fine, and seemed to go through some work (although I got frequent error messages and traceback for pytools.prefork.ExecError: error invoking 'nvcc --version': [Errno 2] No such file or directory).

Sorry about that. We don't do enough testing for CPU-only mode (it's too slow to be of any use in production). Should be fixed now.

The performance tests did not complete, with the error

Traceback (most recent call last):
File "run_and_plot_lysozome_case.py", line 249, in
main()
File "run_and_plot_lysozome_case.py", line 245, in main
generate_plot(compiled_results, filetype='pdf')
File "run_and_plot_lysozome_case.py", line 85, in generate_plot
pyg_ext = richardson_extrapolation(res)
File "run_and_plot_lysozome_case.py", line 60, in richardson_extrapolation
esolv = compiled_results['E_solv_kJ']
KeyError: 'E_solv_kJ'

We catch this exception now.

and the regression tests also led to an error:

Traceback (most recent call last):
File "run_all_regression_tests.py", line 26, in
except FileNotFoundError:
NameError: name 'FileNotFoundError' is not defined

Ugh, Python 2 screws me again. Fixed.

Finally, some minor comments:

Minor comments:

It might be nice to give a recipe to create a conda environment, for those of us that mainly use Python 3 😃

Done. I didn't include PyCUDA in the environment since it isn't super reliable when installing from conda since it makes assumptions about where the cuda installation resides. On a side note, Python 3 version is just about ready to go.

Running the regression (and I assume performance) tests requires matplotlib, which wasn't mentioned as a dependency

Fixed. My bad.

I noticed that the paper author list doesn't completely match the authors listed in the LICENSE, which may or may not be important.

Thanks. Updated the license with the new contributors.

Thanks again, @kyleniemeyer !

from joss-reviews.

@arfon, could we prevail upon you to regenerate the PDF?

Your wish... 10.21105.joss.00043.pdf

from joss-reviews.

Great improvements! The performance and regression tests now run fine for me, and the paper changes look good.

I did run into an issue with the performance test: I was running this on a headless server, and when the test finished I got an error that seems to be the code attempting to display something:

Traceback (most recent call last):
  File "run_and_plot_lysozome_case.py", line 254, in <module>
    main()
  File "run_and_plot_lysozome_case.py", line 250, in main
    generate_plot(compiled_results, filetype='pdf')
  File "run_and_plot_lysozome_case.py", line 93, in generate_plot
    pyplot.figure(figsize=(3, 2), dpi=80)
  File "/home/kylen/anaconda3/envs/python2/lib/python2.7/site-packages/matplotlib/pyplot.py", line 527, in figure
    **kwargs)
  File "/home/kylen/anaconda3/envs/python2/lib/python2.7/site-packages/matplotlib/backends/backend_qt4agg.py", line 46, in new_figure_manager
    return new_figure_manager_given_figure(num, thisFig)
  File "/home/kylen/anaconda3/envs/python2/lib/python2.7/site-packages/matplotlib/backends/backend_qt4agg.py", line 53, in new_figure_manager_given_figure
    canvas = FigureCanvasQTAgg(figure)
  File "/home/kylen/anaconda3/envs/python2/lib/python2.7/site-packages/matplotlib/backends/backend_qt4agg.py", line 76, in __init__
    FigureCanvasQT.__init__(self, figure)
  File "/home/kylen/anaconda3/envs/python2/lib/python2.7/site-packages/matplotlib/backends/backend_qt4.py", line 68, in __init__
    _create_qApp()
  File "/home/kylen/anaconda3/envs/python2/lib/python2.7/site-packages/matplotlib/backends/backend_qt5.py", line 138, in _create_qApp
    raise RuntimeError('Invalid DISPLAY variable')
RuntimeError: Invalid DISPLAY variable

I assume this wouldn't be a problem if this was on a local machine. However, the behavior I would expect is to create the plot (replacing time_lys.png I assume) regardless of whether something can be shown on screen.

from joss-reviews.

@labarba Got it—that makes sense, and now I do remember the discussion on DOI.

from joss-reviews.

Lastly, it looks like the documentation site (http://barbagroup.github.io/pygbe/) is now down.

from joss-reviews.

Awesome, that worked perfectly for me.

I'm happy with the submission at this point. @sobolevnrm, @keith923, and @lizutah: are you also reviewing it?

from joss-reviews.

Awesome, that worked perfectly for me.

👍

@gforsyth - could you add a link to the software archive (e.g. Zenodo/figshare) in this thread? I can then move forward with accepting this submission.

from joss-reviews.

Hey @arfon, I'm happy to. Quick question re protocol: Is it better to do a minor version bump with the post-review changes and use that or should I delete and re-tag and then submit that to Zenodo?

from joss-reviews.

Hey @arfon, I'm happy to. Quick question re protocol: Is it better to do a minor version bump with the post-review changes and use that or should I delete and re-tag and then submit that to Zenodo?

I think the minor version bump is probably better.

from joss-reviews.

I would like to wait for @sobolevnrm, @keith923, and @lizutah to reply whether they are providing some additional review. It would be great to have another seal of approval before acceptance.

from joss-reviews.

I'm afraid that I can't get PyCuda to build on OS X El Capitan. It's looking for 64 bit CUDA libs, and they don't exist in CUDA 7.5, or 6.0. I'd need to switch to a Linux VM, and I'm not sure that CUDA will properly use the GPU in there.

from joss-reviews.

@keith923 -- possible fix for El Capitan and PyCUDA (sorry, no mac to test it on here):

python configure.py --cuda-root=/usr/local/cuda --ldflags="-F/Library/Frameworks -framework CUDA" --cxxflags="-arch x86_64"
make
sudo make install

from joss-reviews.

Thanks @gforsyth. That did help, and I ran into another error that made me look at the problem a bit harder. Short story is that NVIDIA have a prefpane for updating the CUDA driver, but one needs to update the SDK manually. I hopefully won't be much longer.

from joss-reviews.

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.2)?

Functionality

• Installation: Does installation proceed as outlined in the documentation?
• Functionality: Have the functional claims of the software been confirmed?
  Mostly. All the examples worked. @keith923 is posting errors he found in the regression tests
• Performance: Have any performance claims of the software been confirmed?
  Yes, The GPU performance is obvious and the code executes quickly.

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).
  Yes. Pointers to tests and examples exist in the source tree.
• 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

Some notes:

1. Because the code is in github, it’s pretty clear how users can contribute to the software. However there are instructions for contribution.
2. I do not see a mailing list or contact for questions, although github provides a mechanism for this
3. Same as 2.

Other:
“API documentation” isn’t being used correctly. This is really the “User Guide.” The term “API Guide” is used in installation.htmp and in the README.md

The actual API should be a list of modules and description of the code (more like the “Indices and Tables”). You could also call this the “Developer Documentation.”

“Show source” link is broken on left side of page.

Additionally http://barbagroup.github.io/pygbe/ needs a home button. Once you start clicking on links, it’s hard to get back to the main page and the menu in the left changes so you forget how to get back to where you came from.**

Software paper

• Authors: Does the paper.md file include a list of authors with their affiliations?
  Seems like the affiliations could be combined when they are the same?
• 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)?

from joss-reviews.

Running the regression tests I found the following errors.

The following tests:

• Config file: ./pygbe/tests/regression_tests/input_files/lys_8.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/lys.param
• Config file: ./pygbe/tests/regression_tests/input_files/molecule_stern_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param
• Config file: ./pygbe/tests/regression_tests/input_files/neumann_surface_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param
• Config file: ./pygbe/tests/regression_tests/input_files/twosphere_dirichlet_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param
• Config file: ./pygbe/tests/regression_tests/input_files/twosphere_neumann_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param
• Config file: ./pygbe/tests/regression_tests/input_files/twosphere_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param

erred with:

Traceback (most recent call last):
  File "lysozyme.py", line 123, in <module>
    main()
  File "lysozyme.py", line 33, in main
    N, iterations, Esolv, Esurf, Ecoul, Time = run_regression(mesh, test_name, problem_folder, param)
  File "/Users/d3x923/Documents/projects/pygbe/tests/regression_tests/regression.py", line 133, in run_regression
    '{}'.format(problem_folder),], return_output_fname=True)
  File "/usr/local/lib/python2.7/site-packages/PyGBe-0.2-py2.7-macosx-10.11-x86_64.egg/pygbe/main.py", line 340, in main
    dataTransfer(surf_array, field_array, ind0, param, kernel)
  File "/usr/local/lib/python2.7/site-packages/PyGBe-0.2-py2.7-macosx-10.11-x86_64.egg/pygbe/gpuio.py", line 58, in dataTransfer
    s].ycSort.astype(REAL)))
  File "/usr/local/lib/python2.7/site-packages/pycuda/gpuarray.py", line 975, in to_gpu
    result = GPUArray(ary.shape, ary.dtype, allocator, strides=_compact_strides(ary))
  File "/usr/local/lib/python2.7/site-packages/pycuda/gpuarray.py", line 209, in __init__
    self.gpudata = self.allocator(self.size * self.dtype.itemsize)
pycuda._driver.MemoryError: cuMemAlloc failed: out of memory

And these tests:

• Config file: ./pygbe/tests/regression_tests/input_files/molecule_dirichlet_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param
• Config file: ./pygbe/tests/regression_tests/input_files/molecule_neumann_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param
• Config file: ./pygbe/tests/regression_tests/input_files/dirichlet_surface_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param
• Config file: ./pygbe/tests/regression_tests/input_files/molecule_single_32K.config
  Parameter file: ./pygbe/tests/regression_tests/input_files/sphere_fine.param

erred with:

Traceback (most recent call last):
  File "molecule_dirichlet.py", line 76, in <module>
    main()
  File "molecule_dirichlet.py", line 28, in main
    mesh, test_name, problem_folder, param)
  File "/Users/d3x923/Documents/projects/pygbe/tests/regression_tests/regression.py", line 133, in run_regression
    '{}'.format(problem_folder),], return_output_fname=True)
  File "/usr/local/lib/python2.7/site-packages/PyGBe-0.2-py2.7-macosx-10.11-x86_64.egg/pygbe/main.py", line 353, in main
    ind0)
  File "/usr/local/lib/python2.7/site-packages/PyGBe-0.2-py2.7-macosx-10.11-x86_64.egg/pygbe/matrixfree.py", line 900, in generateRHS_gpu
    ind0, timing, kernel)
  File "/usr/local/lib/python2.7/site-packages/PyGBe-0.2-py2.7-macosx-10.11-x86_64.egg/pygbe/projection.py", line 142, in project
    param, LorY, timing, kernel)
  File "/usr/local/lib/python2.7/site-packages/PyGBe-0.2-py2.7-macosx-10.11-x86_64.egg/pygbe/tree/FMMutils.py", line 901, in M2P_gpu
    toc.synchronize()
pycuda._driver.LaunchError: cuEventSynchronize failed: the launch timed out and was terminated

from joss-reviews.

@keith923 : What CUDA card are you using?

from joss-reviews.

Thanks, @lizutah -- I'm working on fixing up the docs as you suggested.

@keith923 -- those all stem from memory issues, I believe. The cards we test on all have 5+ GB of memory and I think the larger meshes are causing problems on your card.

If you edit tests/regression_tests/regression.py as follows it should prevent those errors. Sorry for the trouble!

diff --git a/tests/regression_tests/regression.py b/tests/regression_tests/regression.py
index db05284..08c7553 100644
--- a/tests/regression_tests/regression.py
+++ b/tests/regression_tests/regression.py
@@ -14,8 +14,8 @@ ESURF_REGEX = re.compile('[^: ]Esurf = (\-*\d*\.\d*)\ kcal\/mol')
 ECOUL_REGEX = re.compile('[^: ]Ecoul = (\-*\d*\.\d*)\ kcal\/mol')
 TIME_REGEX = re.compile('Time = (\-*\d*\.\d*)\ s')

-mesh = ['500', '2K', '8K', '32K']
-lysozome_mesh = ['1','2','4','8']
+mesh = ['500', '2K', '8K']
+lysozome_mesh = ['1','2','4']

 def picklesave(test_outputs):
     with open('tests','w') as f:

from joss-reviews.

@labarba my Mac has a GeForce GT 750M with only 2GB of RAM.
@gforsyth while it makes sense to not try to run with the finer meshes, it might be nice if the software caught the exception and reported the problem to the user. If it were me, I might even consider printing a warning and then using a coarser mesh. Perhaps after asking the user if they'd like to proceed like that.

from joss-reviews.

@keith923 -- most definitely. I have a patch in the works to do just that.

from joss-reviews.

@lizutah -- I've pushed documentation fixes as you suggested.

@keith923 -- we now catch both out of memory and synchronize errors with human readable error messages

Thanks both for your comments and please let us know if there's anything else that needs doing.

from joss-reviews.

To @sobolevnrm, @keith923, and @lizutah --- Thank you for the comments that already have made us improve the code and make things better for potential users. We have one "accept" decision by a reviewer (@kyleniemeyer). Are any of you ready to give your thumbs up? Anything else you would like us to do before you are happy?

from joss-reviews.

Yes, happy to give a 👍!

from joss-reviews.

@labarba - seems like we're ready to move forward on this 🎉

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.

from joss-reviews.

Thanks for the review @kyleniemeyer!

@gforsyth @labarba your paper is now accepted and your DOI is http://dx.doi.org/10.21105/joss.00043 🚀 🎉 💥

from joss-reviews.

@whedon check repository

from joss-reviews.

Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.97 s (106.1 files/s, 102434.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
GLSL                            17              0              0          74975
Python                          54           2772           6034           7506
SWIG                             5            290            514           2892
C++                              4            307             49           1920
Markdown                         5            225              0            553
reStructuredText                11            245            100            448
make                             1             29              6            181
TeX                              1             20              4            170
JSON                             1              0              0             44
Dockerfile                       1              9             24             33
YAML                             1              4              2             29
HTML                             1              0              0              2
Jupyter Notebook                 1              0             71              0
-------------------------------------------------------------------------------
SUM:                           103           3901           6804          88753
-------------------------------------------------------------------------------


Statistical information for the repository '43' was gathered on 2020/06/20.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Anush Krishnan                   1             1              1            0.00
Christopher Cooper              32         17811           4302           14.88
Gil Forsyth                    229         52357          53361           71.14
Kyle Niemeyer                    1             3              1            0.00
Natalia C. Clementi            290          8757           6141           10.03
Naty Clementi                   71          2201            883            2.08
cdcooper                         2            91             15            0.07
cdcooper84                       8          2352            295            1.78
hache                            1             9              9            0.01
ncclementi                       1             9              6            0.01

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Christopher Cooper           17            0.1         68.4                5.88
Gil Forsyth                7456           14.2         45.1                9.95
Kyle Niemeyer                 3          100.0         33.1                0.00
Natalia C. Clementi        4181           47.7         40.5               11.07
cdcooper                   6918         7602.2         68.5                3.09
cdcooper84                    4            0.2         39.6                0.00
hache                         9          100.0          0.0                0.00

from joss-reviews.