[REVIEW]: High-performance xPU Stencil Computations in Julia

whedon commented 10 months ago

Submitting author: !--author-handle-->@omlins@fcdimitr<!--end-editor-- Reviewers: @georgebisbas, @bgeihe Archive: Pending

Status

Status badge code:

HTML: <a href="https://proceedings.juliacon.org/papers/c28f9f11e3123ebaa856aeca43962b5f"><img src="https://proceedings.juliacon.org/papers/c28f9f11e3123ebaa856aeca43962b5f/status.svg"></a>
Markdown: [![status](https://proceedings.juliacon.org/papers/c28f9f11e3123ebaa856aeca43962b5f/status.svg)](https://proceedings.juliacon.org/papers/c28f9f11e3123ebaa856aeca43962b5f)

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) by leaving comments 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

@georgebisbas & @bgeihe, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @fcdimitr know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @georgebisbas

Conflict of interest

[x] As the reviewer I confirm that I have read the JuliaCon conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JuliaCon code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Authorship: Has the submitting author (@omlins) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] 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

Paper format

[x] Authors: Does the paper.tex file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
[x] Page limit: Is the page limit for extended abstracts respected by the submitted document?

Content

[x] Context: is the scientific context motivating the work correctly presented?
[x] Methodology: is the approach taken in the work justified, presented with enough details and reference to reproduce it?
[ ] Results: are the results presented and compared to approaches with similar goals?

Review checklist for @bgeihe

Conflict of interest
[x] As the reviewer I confirm that I have read the JuliaCon conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JuliaCon code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Authorship: Has the submitting author (@omlins) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] 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

Paper format

[x] Authors: Does the paper.tex file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
[x] Page limit: Is the page limit for extended abstracts respected by the submitted document?

Content

[x] Context: is the scientific context motivating the work correctly presented?
[x] Methodology: is the approach taken in the work justified, presented with enough details and reference to reproduce it?
[x] Results: are the results presented and compared to approaches with similar goals?

whedon commented 10 months ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @georgebisbas, @bgeihe it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/JuliaCon/proceedings-review) 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:

Set yourself as 'Not watching' https://github.com/JuliaCon/proceedings-review:

watching

You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf

whedon commented 10 months ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.06 s (1239.6 files/s, 187708.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           62            887            552           8623
Markdown                         1            130              0            435
YAML                             2              3              2             55
TOML                             6             10              0             54
-------------------------------------------------------------------------------
SUM:                            71           1030            554           9167
-------------------------------------------------------------------------------

Statistical information for the repository '04166151f14578eb224e9fe1' was
gathered on 2023/11/06.
No commited files with the specified extensions were found.

whedon commented 10 months ago

PDF failed to compile for issue #138 with the following error:

 Can't find any papers to compile :-(

fcdimitr commented 10 months ago

@whedon generate pdf from branch JuliaConProceeding2022

whedon commented 10 months ago

Attempting PDF compilation from custom branch JuliaConProceeding2022. Reticulating splines etc...

whedon commented 10 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

fcdimitr commented 10 months ago

@whedon check references

fcdimitr commented 10 months ago

@whedon check references from branch JuliaConProceeding2022

whedon commented 10 months ago

Attempting to check references... from custom branch JuliaConProceeding2022

whedon commented 10 months ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/SC41405.2020.00062 is OK
- ??? is OK
- 10.21105/jcon.00068 is OK
- 10.5194/gmd-15-5757-2022 is OK

MISSING DOIs

- 10.1137/141000671 may be a valid DOI for title: Julia: A fresh approach to numerical computing
- 10.1109/tpds.2018.2872064 may be a valid DOI for title: Effective extensible programming: unleashing Julia on GPUs

INVALID DOIs

- None

fcdimitr commented 10 months ago

@whedon remove @sloede as reviewer

whedon commented 10 months ago

OK, @sloede is no longer a reviewer

fcdimitr commented 10 months ago

@whedon add @bgeihe as reviewer

whedon commented 10 months ago

OK, @bgeihe is now a reviewer

fcdimitr commented 10 months ago

@whedon re-invite @bgeihe as reviewer

whedon commented 10 months ago

The reviewer already has a pending invite.

@bgeihe please accept the invite by clicking this link: https://github.com/JuliaCon/proceedings-review/invitations

fcdimitr commented 10 months ago

@bgeihe thank you for volunteering as a reviewer. You can find more details on the JuliaCon review process here. Feel free to ask me any questions you have.

benegee commented 10 months ago

Review

ParallelStencil.jl is a package which allows to implement efficient algorithms based on stencil computations. Its special feature is being architecture-agnostic, which means that the same code can be used for prototyping and high performance computations on either CPUs or GPUs.

The extended abstract is written clearly and concisely. Motivation is given, the approach for implementation is sketched, and numerical examples for 3D heat diffusion as well as nonlinear problems are presented. The impressive results demonstrate the claimed near optimal performance.

I have linked three issues with suggestions as to minor modifications. Once these have been addressed, I think the extended abstract should be accepted for publication.

whedon commented 10 months ago

:wave: @georgebisbas, please update us on how your review is going (this is an automated reminder).

georgebisbas commented 10 months ago

ParallelStencil.jl offers a high-level textbook-math-close interface and abstractions for solving PDEs for the HPC solution of PDEs. It can also support explicit stencil definition and computation. Users can benefit from seamlessly transitioning the high-level code to target both CPUs and GPUs, with only minor to no edits in the source code.

This work has a high overlap with https://github.com/JuliaCon/proceedings-review/issues/137 I propose that they be merged into a paper covering both packages. However, this is not a firm requirement. Since ParallelStencil.jl does not claim distributed-memory parallelism benefits, mentions to DMP should be removed to reduce the overlap. For example: Abstract: …and multiple dispatch enable writing a single code that is suitable for both productive proto typing on a single CPU thread and production runs on multi-GPU or CPU workstations or supercomputers….
Another suggestion is to avoid using the term xPU in the title. In the recent rise of “exotic” accelerators like Google’s TPU, Graphcore’s IPU, or expected QPUs, it probably would not harm just to use: High-performance Stencil Computations in Julia And then in the abstract just make it clear that this work is about CPU and GPU.
The package's functionality is confirmed, and the installation, testing and use documentation is present and well-defined.
The paper is well-written, clearly presenting the motivation and example usage of the package. This work also benefits from the math-close interface, which is only sometimes provided in the stencil-specific scientific community. Projects like OpenSBLI, Devito, and Exastencils provide such interfaces, primarily not in Julia. The paper would probably benefit by slightly “upgrading” the reference to the symbolic interface, as this helps interdisciplinary scientists take advantage of HPC solvers without the need to entangle with low-level HPC optimisations.
The diffusion example presented is well-written. Minor comments (not aiming to be addressed, but possibly interesting points for future work and further discussion): Could the use of both “dx” and “_dx” possibly be avoided? As far as I understand, it only helps to reduce the text in equations present in the parallel loop (lines 5:12) while only adding more complexity (with no comments in line 26). Additionally, T2 in line 30 is the stencil computation's second buffer. Does this mean the user would need to add additional copies if the PDE solvers require a higher accuracy order in time? (e.g. time discretisation order in acoustic wave propagation is 2 (needing T, T2, T3?))
Should Figure 1 be presented as a Listing 1?
Should Figure 2 be split into two subfigures?
Performance evaluation: Linked issue in the package repository.

Assuming the above concerns are addressed, this should be accepted for publication

omlins commented 9 months ago

@bgeihe : Thank you very much for the thorough review. We are working on addressing the reviews, including the issues you have opened.

omlins commented 9 months ago

@georgebisbas : Thank you very much for the thorough review. We are working on addressing the reviews. In the following you can find replies to two of the issues that you raised. We will respond to the remaining issues alongside with the improvements of the manuscript.

This work has a high overlap with https://github.com/JuliaCon/proceedings-review/issues/137 I propose that they be merged into a paper covering both packages. However, this is not a firm requirement. Since ParallelStencil.jl does not claim distributed-memory parallelism benefits, mentions to DMP should be removed to reduce the overlap. For example: Abstract: …and multiple dispatch enable writing a single code that is suitable for both productive proto typing on a single CPU thread and production runs on multi-GPU or CPU workstations or supercomputers….

Thank you for pointing out the overlaps concerning distributed memory parallelism. We will remove it as much as possible in order to publish the packages ParallelStencil.jl and ImplicitGlobalGrid.jl in separate papers. We would like to keep the format of one paper per package, given that they have a different focus and can be used independently.

Performance evaluation: Linked issue in the package repository.

We have replied to this issue directly where it was opened.

fcdimitr commented 6 months ago

Hi @omlins! I'm just checking in; what is the status of this? Thank you!

fcdimitr commented 6 months ago

@editorialbot generate pdf

fcdimitr commented 6 months ago

@editorialbot commands

editorialbot commented 6 months ago

Hello @fcdimitr, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for branch
@editorialbot set juliacon-paper as branch

# Reject paper
@editorialbot reject

# Withdraw paper
@editorialbot withdraw

# Invite an editor to edit a submission (sending them an email)
@editorialbot invite @(.*) as editor

# Run checks and provide information on the repository and the paper file
@editorialbot check repository

# Check the references of the paper for missing DOIs
@editorialbot check references

# Generates the pdf paper
@editorialbot generate pdf

# Accept and publish the paper
@editorialbot accept

# Update data on an accepted/published paper
@editorialbot reaccept

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers

editorialbot commented 6 months ago

:warning: An error happened when generating the pdf. Paper file not found.

fcdimitr commented 6 months ago

@editorialbot set JuliaConProceeding2022 as branch

editorialbot commented 6 months ago

Done! branch is now JuliaConProceeding2022

fcdimitr commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

:warning: An error happened when generating the pdf.

lucaferranti commented 6 months ago

The error above is expecetd and known. The new infrastructure is mainly in place, the only piece missing is the generation of the xml file, which is what the error is comlaining about

fcdimitr commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

:warning: An error happened when generating the pdf.

fcdimitr commented 6 months ago

The error might be due to missing DOIs. @lucaferranti is my understanding correct?

@omlins could you add the missing DOIs and try to generate the PDF again?

Thank you!

omlins commented 6 months ago

@fcdimitr : please excuse the delay for the reply. We aim to finalize this extended abstract hopefully this and next week.

I have updated the bibliography file; I think now it should be able to generate the PDF.

lucaferranti commented 6 months ago

The error might be due to missing DOIs. @lucaferranti is my understanding correct?

sorry for the delay in replying, but I think it's correct, ping me if htere are still issues with editorialbot

fcdimitr commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

fcdimitr commented 4 months ago

Hi @omlins! I'm just checking in; what is the status of this? Thank you!

omlins commented 2 months ago

We aim to address the remaining issues in the manuscript in the next couple of days. Please excuse the delay.

omlins commented 2 months ago

Verifying that generating PDF still works before pushing changes:

omlins commented 2 months ago

@editorialbot generate pdf

editorialbot commented 2 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

luraess commented 2 months ago

@editorialbot generate pdf

editorialbot commented 2 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

omlins commented 1 month ago

@editorialbot generate pdf

editorialbot commented 1 month ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

omlins commented 1 month ago

Dear reviewers, we have improved the manuscript, addressing all the issues raised. @georgebisbas @bgeihe

omlins commented 1 month ago

@benegee

JuliaCon / proceedings-review