-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: PyGBe: Python, GPUs and Boundary elements for biomolecular electrostatics #43
Comments
/ 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 Reviewer instructions
Any questions, please ask for help by commenting on this issue! 🚀 |
✋ I'm happy to review this! |
@kyleniemeyer - the paper has just been updated for this submission. |
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... |
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.
I also have some comments on the paper:
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 The performance tests did not complete, with the error
and the regression tests also led to an error:
Finally, some minor comments: Minor comments:
|
Thanks, @kyleniemeyer ! We'll get on fixes for the pain points. |
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: 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. |
@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. |
The README is now duplicated in the github-pages documentation, along with the contribution guide and the input files reference.
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).
Thanks for the kick in the rear -- PyGBe now works with Cuda <= 7.5
I believe we have fixed this now.
We have tried to address this with a few application examples.
Added. Thanks. @arfon, could we prevail upon you to regenerate the PDF?
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.
We catch this exception now.
Ugh, Python 2 screws me again. Fixed.
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.
Fixed. My bad.
Thanks. Updated the license with the new contributors. Thanks again, @kyleniemeyer ! |
Your wish... 10.21105.joss.00043.pdf |
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:
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 |
@labarba Got it—that makes sense, and now I do remember the discussion on DOI. |
Lastly, it looks like the documentation site (http://barbagroup.github.io/pygbe/) is now down. |
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.
Yikes. Not sure what happened there, but it's back up now. Thanks! |
Awesome, that worked perfectly for me. I'm happy with the submission at this point. @sobolevnrm, @keith923, and @lizutah: are you also reviewing it? |
👍 @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. |
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. |
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. |
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. |
@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 |
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. |
Conflict of interest
General checks
Functionality
Documentation
Some notes:
Other: 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
|
Running the regression tests I found the following errors. The following tests:
erred with:
And these tests:
erred with:
|
@keith923 : What CUDA card are you using? |
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 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: |
@labarba my Mac has a GeForce GT 750M with only 2GB of RAM. |
@keith923 -- most definitely. I have a patch in the works to do just that. |
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? |
Yes, happy to give a 👍! |
@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. |
Version bump to 2.1 with all revisions. 10.5281/zenodo.60474 🎉🎉🎉 |
Thanks for the review @kyleniemeyer! @gforsyth @labarba your paper is now accepted and your DOI is http://dx.doi.org/10.21105/joss.00043 🚀 🎉 💥 |
Thank you also to reviewers # 2 and # 3: @keith923 and @lizutah, and marshalling by @sobolevnrm. |
@whedon check repository |
|
Submitting author: @labarba (Lorena A. Barba)
Repository: https://github.com/barbagroup/pygbe
Version: 2.1
Editor: @arfon
Reviewer: @kyleniemeyer
Archive: 10.5281/zenodo.60474
Status
Status badge code:
Reviewer questions
Conflict of interest
General checks
Functionality
Documentation
Software paper
Paper PDF: 10.21105.joss.00043.pdf
paper.md
file include a list of authors with their affiliations?The text was updated successfully, but these errors were encountered: