[REVIEW]: AmgXWrapper: An interface between PETSc and the NVIDIA AmgX library

[whedon]

Submitting author: @piyueh (Pi-Yueh Chuang)
Repository: https://github.com/barbagroup/AmgXWrapper
Version: v1.0
Editor: @danielskatz
Reviewer: @jedbrown
Archive: 10.5281/zenodo.852471

Status

Status badge code:

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

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 questions

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 (v1.0)?
• Authorship: Has the submitting author (@piyueh) made major contributions to the software?

Functionality

• Installation: Does installation proceed as outlined in the documentation?
• Functionality: Have the functional claims of the software been confirmed?
• Performance: Have any performance claims of the software been confirmed?

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 for JOSS. @jedbrown 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 as reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all JOSS 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

[danielskatz]

@jedbrown - this is ready for you to review. You should review the reviewer instructions on http://joss.theoj.org/about. Let me know if you have any problems.

[labarba]

@jedbrown : I have enquired with NVIDIA about the trial license of AmgX. They are sure going to open source it, but it's undergoing code review right now.

[danielskatz]

@jedbrown - I talked to @labarba last week, and I believe she's working on getting NVIDIA to provide a license for AmgX for you.

[jedbrown]

Joe Eaton sent me a license a few days ago. Thanks. "Daniel S. Katz" <[email protected]> writes:

…

@jedbrown - I talked to @labarba last week, and I believe she's working on getting NVIDIA to provide a license for AmgX for you. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #280 (comment)

[danielskatz]

@jedbrown - thanks. let me know if you have any problems in the review.

[danielskatz]

@jedbrown - please let us know how this is going

[labarba]

@jedbrown Do you have access to a GPU system to run this? Let us know if you need anything.

[danielskatz]

I pinged @jedbrown via email

[danielskatz]

We seem to have lost @jedbrown . @labarba, maybe we need to find another reviewer. Do you have more suggestions?

[jedbrown]

Sorry, I've been delayed by our newborn, but will be able to finish this soon. "Daniel S. Katz" <[email protected]> writes:

…

We seem to have lost @jedbrown . @labarba, maybe we need to find another reviewer. Do you have more suggestions? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #280 (comment)

[danielskatz]

Ok, great to hear - and congrats!

[danielskatz]

@jedbrown - any progress? Or should we say this isn't going to work and find another reviewer? (If so, any suggestions you have would be very welcome.)

[jedbrown]

I'm installing now. Sorry about the delays. "Daniel S. Katz" <[email protected]> writes:

…

@jedbrown - any progress? Or should we say this isn't going to work and find another reviewer? (If so, any suggestions you have would be very welcome.) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #280 (comment)

[jedbrown]

I've filed issues in AmgXWrapper for the technical concerns that came up during my review and they are either resolved, in the process of being resolved, or not blocking issues.

The most significant editorial comment (not a blocking issue) is in barbagroup/AmgXWrapper#17 which proposes a much more useful way for the software to be organized and distributed. Distributing as a plugin providing a PETSc PC would immediately allow AmgX to be used by any software that solves systems using PETSc.

[danielskatz]

@jedbrown - Can you check off the items that you think are done in the list above?

And make it clear what else you think is needed to check off the remaining items, so that the authors know what they need to do vs what they could do later.

[jedbrown]

@danielskatz Done. Some comments:

• Performance: I don't have an equivalent system and can't verify specific performance claims, but I do see respectable performance from AmgX Classical.
• Installation: The instructions are adequate, but there is an implicit dependency on an antique version of GCC (via CUDA-6.5) and there is no package manager solution (though this is a sufficiently mixed stack that no single method would be platform independent). It would help to describe the variables CUDA_DIR and AMGX_DIR since (to my knowledge) these are not standard names so it can be unclear whether this should be a normal prefix (libraries in $AMGX_DIR/lib) or more complete path.
• Example: The example is very much a toy problem, but is relatively easy to understand. There is not currently an example of using the compiled library versus compiling the sources as part of the executable.
• Tests: There are no automated tests, but there is a manual process. See also Some automated test for the library barbagroup/AmgXWrapper#19.
• Community: There is no CONTRIBUTING.md or equivalent (see Add CONTRIBUTING.md barbagroup/AmgXWrapper#20) but the project appears to follow de facto conventions for projects on GitHub.

[danielskatz]

Thanks @jedbrown - I think you've done a nice job of saying where this meets a minimal set of requirements, where improvements could be made (but are not mandatory for JOSS acceptance), and where one change is needed for JOSS acceptance.

[danielskatz]

@piyueh - As you can see from the discussion, there is one change you need to make to allow JOSS to accept this - as discussed in barbagroup/AmgXWrapper#20

[piyueh]

Thank you @jedbrown @danielskatz

I'll work on this now.

[piyueh]

@danielskatz @jedbrown
I've added CONTRIBUTING.md as described in barbagroup/AmgXWrapper#20.

[danielskatz]

@jedbrown - please check the contributing and if it's ok, please check off the item in the checklist above, and then, if everything else seems good, let me know that you recommend the paper be published. Otherwise, let me know what else you think needs to be done.

[jedbrown]

Looks good to me.

[labarba]

Yay!! We have full checkmarks!

Thank you, @jedbrown —we received lots of good, deep, constructive criticism from your review.

[danielskatz]

Thanks for the good review @jedbrown

@arfon, this is ready to go to you now for the next steps towards acceptance and publishing.

[arfon]

@piyueh - could you please merge this change to the paper metadata: barbagroup/AmgXWrapper#21

Also, 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.

[piyueh]

Hi @arfon
Heere is the Zenodo DOI: 10.5281/zenodo.852471

Thank you!

[arfon]

@whedon set 10.5281/zenodo.852471 as archive

[whedon]

OK. 10.5281/zenodo.852471 is the archive.

[arfon]

@jedbrown many thanks for your review and to @danielskatz for editing this submission ✨

@piyueh - your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00280 ⚡️ 🚀 💥

[jedbrown]

https://doi.org is preferred, but the earlier syntax dx.doi.org remains fully supported

https://www.doi.org/doi_handbook/3_Resolution.html

[danielskatz]

Good point. @arfon, we should change all of these that we generate automatically

[arfon]

Good point. @arfon, we should change all of these that we generate automatically

Yep, I'd managed to forget about that. I've added it to the backlog here openjournals/joss#322

[labarba]

I see this as accepted 12 days ago, but cannot find it published. Is it in some limbo? (currently updating my CV for promotion application!)

[arfon]

@labarba see #280 (comment)

[labarba]

Great news! NVIDIA has now released AmgX with an open source license:
https://github.com/NVIDIA/AMGX

[labarba]

@whedon check repository

[whedon]

Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.07 s (791.2 files/s, 104375.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             17            891            448           2071
CMake                           12            257            352           1070
Markdown                        10            176              0            534
C/C++ Header                    11            206            592            212
JSON                             1              0              0             30
TeX                              1              1              0             20
-------------------------------------------------------------------------------
SUM:                            52           1531           1392           3937
-------------------------------------------------------------------------------


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

Author                     Commits    Insertions      Deletions    % of changes
Jed Brown                        1            12              8            0.13
Markus Hrywniak                  1            90             16            0.67
Pi-Yueh                          2          2637             51           17.00
Pi-Yueh Chuang                  38          7391           5563           81.93
mesnardo                         1            16             28            0.28

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
Jed Brown                    12          100.0         23.4                0.00
Markus Hrywniak              90          100.0          0.1               26.67
Pi-Yueh Chuang             4364           59.0         30.4               24.20
mesnardo                     14           87.5         22.3                0.00