search

openjournals / joss-reviews

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

[REVIEW]: HealpixMPI.jl: an MPI-parallel implementation of the Healpix tessellation scheme in Julia #6467

Closed editorialbot closed 5 months ago

editorialbot commented 7 months ago

Submitting author: !--author-handle-->@LeeoBianchi<!--end-author-handle-- (Leo Alessandro Bianchi) Repository: https://github.com/LeeoBianchi/HealpixMPI.jl Branch with paper.md (empty if default branch): JOSS-paper Version: v1.0.0 Editor: !--editor-->@prashjha<!--end-editor-- Reviewers: @marcobonici, @baxmittens Archive: 10.5281/zenodo.11192548

Status

status

Status badge code:

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

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

@marcobonici & @baxmittens, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @prashjha 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 ✨

Checklists

📝 Checklist for @marcobonici

📝 Checklist for @baxmittens

editorialbot commented 7 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 7 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1051/0004-6361/202346414 is OK
- 10.1086/427976 is OK
- 10.1086/425219 is OK
- 10.1051/0004-6361/201321494 is OK
- 10.21105/jcon.00068 is OK

MISSING DOIs

- No DOI given, and none found for title: Healpix.jl: Julia-only port of the HEALPix library
- No DOI given, and none found for title: DUCC

INVALID DOIs

- None
editorialbot commented 7 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.03 s (1289.2 files/s, 114571.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           22            399            493           1425
Markdown                         8            187              0            626
YAML                             5              8             12            106
TeX                              1              7              0             83
TOML                             2              6              0             25
-------------------------------------------------------------------------------
SUM:                            38            607            505           2265
-------------------------------------------------------------------------------

Commit count by author:

   115  LeeoBianchi
    39  Leo Alessandro Bianchi
     2  Leo A. Bianchi
editorialbot commented 7 months ago

Paper file info:

📄 Wordcount for paper.md is 1462

✅ The paper includes a Statement of need section

editorialbot commented 7 months ago

License info:

🟡 License found: GNU General Public License v2.0 (Check here for OSI approval)

editorialbot commented 7 months ago

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

prashjha commented 7 months ago

Dear @marcobonici and @baxmittens, please read the first couple of comments in this thread and create your review checklist if you have not done so already. You can read the reviewer guidelines here. Also, you can browse the closed "REVIEW" issues on the "joss-reviews" repository to get some ideas on how to complete the reviews. Thank you!

marcobonici commented 7 months ago

Review checklist for @marcobonici

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

baxmittens commented 7 months ago

Review checklist for @baxmittens

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

baxmittens commented 7 months ago

Hello @LeeoBianchi

is there a way I can confirm the performance claims you have made in this paper? Can you guide me to a script, that I can run on my servers?

Greetz max

LeeoBianchi commented 7 months ago

Hello @LeeoBianchi

is there a way I can confirm the performance claims you have made in this paper? Can you guide me to a script, that I can run on my servers?

Greetz max

Hello @baxmittens,

Sure, here is a script I used to obtain the benchmark timings for the plot in my paper:

using MPI
using Healpix
using HealpixMPI
using BenchmarkTools
using DelimitedFiles
MPI.Init()

comm = MPI.COMM_WORLD
global crank = MPI.Comm_rank(comm)
global csize = MPI.Comm_size(comm)
root = 0

NSIDE = 4096
lmax = 3*NSIDE - 1

#initialize some Healpix data
if crank == root
    test_map = HealpixMap{Float64, RingOrder}(NSIDE)
    test_alm = Alm(lmax, lmax, [ComplexF64(i) for i in 1:numberOfAlms(lmax)])
else
    test_map = nothing
    test_alm = nothing
end

#distribute it into HealpixMPI types
d_map = DMap{RR}(comm)
MPI.Scatter!(test_map, d_map, comm)
d_alm = DAlm{RR}(comm)
MPI.Scatter!(test_alm, d_alm, comm)

#auxiliary optional legeandre coeffs arrays for more efficient SHTs
aux_alm_leg = Array{ComplexF64,3}(undef, (length(d_alm.info.mval), numOfRings(d_map.info.nside), 1)) # loc_nm * tot_nr
aux_map_leg = Array{ComplexF64,3}(undef, d_alm.info.mmax+1, length(d_map.info.rings), 1)

NTs = [2, 4, 8 , 16, 32] #different number of threads to run the benchmarks with.
ba2m = Vector{Float64}(undef, length(NTs)) #result arrays
bm2a = Vector{Float64}(undef, length(NTs)) #result arrays
#run benchmarks
for i in 1:length(NTs)
    println("alm2map benchmark for $csize MPI tasks and $(NTs[i]) C++ threads ...")
    global NT = NTs[i]
    b = @benchmark alm2map!(d_alm, d_map, aux_alm_leg, aux_map_leg; nthreads = NT) samples = 20 seconds = 600 evals = 1
    ba2m[i] = minimum(b.times) * NT * csize / 1e9 #in seconds
    println("adjoint_alm2map benchmark for $csize MPI tasks and $(NTs[i]) C++ threads ...")
    b = @benchmark adjoint_alm2map!(d_map, d_alm, aux_map_leg, aux_alm_leg; nthreads = NT) samples = 20 seconds = 600 evals = 1
    bm2a[i] = minimum(b.times) * NT * csize / 1e9 #in seconds
end
#print results
if crank == root
    outfile = "HpixMPI_a2m_full_Nside$(NSIDE)_$(csize)MPI_$(NT)threads.txt"
    writedlm(outfile, ba2m)
    outfile = "HpixMPI_adj_full_Nside$(NSIDE)_$(csize)MPI_$(NT)threads.txt"
    writedlm(outfile, bm2a)
end

Just drop a message if there is anything unclear and I will try to help you.

Thank you for undertaking this review process :)

Cheers, Leo

danielskatz commented 7 months ago

Hi - as the track editor, I'm just stepping in briefly to suggest that this script might be added to the repo, and pointed to from the README or elsewhere.

baxmittens commented 6 months ago

I have run the script on 1-4 MPI processes

HpixMPI_adj_full_Nside4096_1MPI_32threads.txt 61.425054444 67.324124836 66.981430832 68.79545176 73.325745312

HpixMPI_a2m_full_Nside4096_1MPI_32threads.txt 105.384205176 113.553485328 114.497209256 116.934635904 122.462031616

HpixMPI_adj_full_Nside4096_2MPI_32threads.txt 63.378890184 67.107265688 81.426459472 92.957265344 120.978923648

HpixMPI_a2m_full_Nside4096_2MPI_32threads.txt 106.813835852 114.115288024 129.216200368 150.261070752 170.98185888

HpixMPI_adj_full_Nside4096_3MPI_32threads.txt 64.932011886 70.056360972 83.512326096 102.962028768 151.741963968

HpixMPI_a2m_full_Nside4096_3MPI_32threads.txt 107.305189548 112.57675122 128.54376792 146.44734288 189.411806112

HpixMPI_adj_full_Nside4096_4MPI_32threads.txt 67.3199592 70.524248256 83.624625696 104.480749696 173.429741056

HpixMPI_a2m_full_Nside4096_4MPI_32threads.txt 107.487257936 112.815567168 130.402578976 146.577254784 197.771545088

baxmittens commented 6 months ago
image image
LeeoBianchi commented 6 months ago

Hi @baxmittens, I can see you managed to run the benchmark script.

The plots you have obtained look comparable with mine on the paper, however I can see a quite steep jump between the 24(?) and 32-threads cases, especially in the adjoint transform. This can be due to a few things. I'd be curious to know a few details about your set-up:

Cheers, Leo

baxmittens commented 6 months ago

Hi @LeeoBianchi

I just ran them locally on one node. That is ok, since I do not want to test MPI, right?

My System is

Therefore my timings are, as far as I can see, a little bit faster compared to yours. I dunno if both CPUs have fast access to all the memory. Maybe therefore there is a jump in the wall time?

Greetz, Max

LeeoBianchi commented 6 months ago

I just ran them locally on one node. That is ok, since I do not want to test MPI, right?

Okay I see. Actually the way I conducted the benchmarks was to assign one MPI task to each node, so my benchmark also implied some time being spent for sending data over the cluster structure. That might explain your timings being a bit faster. Your test, however, is a good way to simulate what I have done, yes.

My System is

  • 2 x AMD EPYC 7763 64-Core Processor (á 64 Cores, Hyperthreading deactivated)
  • 4020 GB RAM

Therefore my timings are, as far as I can see, a little bit faster compared to yours. I dunno if both CPUs have fast access to all the memory. Maybe therefore there is a jump in the wall time?

I agree, the jump in the wall time could really look like a memory access matter to me, especially when using all the cores available at the same time.

Let me know if you have any further question to proceed with the review.

Cheers, Leo

baxmittens commented 6 months ago

@LeeoBianchi

Did you notice that I commited a pull request for your paper.md? https://github.com/LeeoBianchi/HealpixMPI.jl/pull/7/commits/a9cac762b87ee55f44652214e7918b3ee09a04d5 I ran Grammarly on your paper. It has found a few commas missing and some other stuff. You can consider accepting some of these suggestions, but you don't have to. I found them quite helpful. But be careful since they were machine-generated.

baxmittens commented 6 months ago

@LeeoBianchi I did not find Community guidelines

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
marcobonici commented 6 months ago

@LeeoBianchi I added a couple of comments, opening github issues as described here. Additionally, would you mind expanding on the state-of-the-art for distributed SHT? I think I saw something based on Jax that was also GPU compatible, but I am not sure. THank you in advance! At the end of this week I should be able to run your code, so I can finish my review.

LeeoBianchi commented 6 months ago

@baxmittens @marcobonici I apologize for the delay in my replies, but things have been quite hectic lately at work. I promise I will get back to this review as soon as possible, hopefully already this weekend. Thank you!

LeeoBianchi commented 6 months ago

@LeeoBianchi I added a couple of comments, opening github issues as described here. Additionally, would you mind expanding on the state-of-the-art for distributed SHT? I think I saw something based on Jax that was also GPU compatible, but I am not sure. THank you in advance! At the end of this week I should be able to run your code, so I can finish my review.

By expanding do you mean to provide some more details here, or to add something in the paper?

LeeoBianchi commented 6 months ago

@LeeoBianchi I did not find Community guidelines

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

I think you're right. I gave for granted the standard GitHub procedures/tools would have been used. Should I specify that on the Readme of the repository?

baxmittens commented 6 months ago

I think you're right. I gave for granted the standard GitHub procedures/tools would have been used. Should I specify that on the Readme of the repository?

I think that's what the JOSS guidelines prescribe. Just take any other Joss-reviewed repository like

https://github.com/gdalle/HiddenMarkovModels.jl

as a reference.

LeeoBianchi commented 6 months ago

I think you're right. I gave for granted the standard GitHub procedures/tools would have been used. Should I specify that on the Readme of the repository?

I think that's what the JOSS guidelines prescribe. Just take any other Joss-reviewed repository like

https://github.com/gdalle/HiddenMarkovModels.jl

as a reference.

Please find the updates to README.md in 0e38b16.

Leo

baxmittens commented 6 months ago

@LeeoBianchi Can you merge the pull request? I can then create an updated version of the paper. Thanks.

LeeoBianchi commented 6 months ago

@LeeoBianchi Can you merge the pull request? I can then create an updated version of the paper. Thanks.

@baxmittens please see my last comment on the pull request review.

baxmittens commented 6 months ago

@LeeoBianchi Sorry, I never did this before in the browser without forking. I tried to change the line. Did it work?

baxmittens commented 6 months ago

@LeeoBianchi Otherwise, you can just accept the pull request an re-edit as necessary.

baxmittens commented 6 months ago

@editorialbot generate pdf

LeeoBianchi commented 6 months ago

@baxmittens no problem :) I approved and merged. We should be good to go on now.

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:

baxmittens commented 6 months ago

Hi - as the track editor, I'm just stepping in briefly to suggest that this script might be added to the repo, and pointed to from the README or elsewhere.

@LeeoBianchi Did you add the script to the repo? Can you point me to it?

LeeoBianchi commented 6 months ago

@baxmittens oops, I had changed everything locally but still haven't pushed since. Done now. You can find the example folder with the script inside. I also mentioned it in the readme.

baxmittens commented 6 months ago

@LeeoBianchi Thank you. I am done now with my list and I think this paper can be accepted if @marcobonici is through with his list, too.

The next step would then be to create a Zenodo archive, but I think that call is to be made by the editor, @prashjha , right?

marcobonici commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

marcobonici commented 5 months ago

I am fine with the paper. The only missing thing from my side, is a citation that I asked @LeeoBianchi to add. After this, we are good to go!

LeeoBianchi commented 5 months ago

@marcobonici you're absolutely right! Will add it asap.

LeeoBianchi commented 5 months ago

I am fine with the paper. The only missing thing from my side, is a citation that I asked @LeeoBianchi to add. After this, we are good to go!

@marcobonici please find my latest comment on the issue and the updated version of the paper to include the reference you suggested.

baxmittens commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

LeeoBianchi commented 5 months ago

I fixed the citation format of the Euclid paper.

LeeoBianchi commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

Kevin-Mattheus-Moerman commented 5 months ago

@prashjha can you pick this one up again? Looks like it may be ready to proceed.

prashjha commented 5 months ago

@editorialbot commands

editorialbot commented 5 months ago

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


# List all available commands
@editorialbot commands

# Add to this issue's reviewers list
@editorialbot add @username as reviewer

# Remove from this issue's reviewers list
@editorialbot remove @username from reviewers

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

# Assign a user as the editor of this submission
@editorialbot assign @username as editor

# Remove the editor assigned to this submission
@editorialbot remove editor

# Remind an author, a reviewer or the editor to return to a review after a 
# certain period of time (supported units days and weeks)
@editorialbot remind @reviewer in 2 weeks

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

# Set a value for version
@editorialbot set v1.0.0 as version

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

# Set a value for repository
@editorialbot set https://github.com/organization/repo as repository

# Set a value for the archive DOI
@editorialbot set 10.5281/zenodo.6861996 as archive

# Mention the EiCs for the correct track
@editorialbot ping track-eic

# 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

# Recommends the submission for acceptance
@editorialbot recommend-accept

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Flag submission with questionable scope
@editorialbot query scope

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

# Creates a post-review checklist with editor and authors tasks
@editorialbot create post-review checklist

# Open the review issue
@editorialbot start review
prashjha commented 5 months ago

@editorialbot check references

editorialbot commented 5 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1051/0004-6361/202346414 is OK
- 10.1086/427976 is OK
- 10.1086/425219 is OK
- 10.1051/0004-6361/201321494 is OK
- 10.21105/jcon.00068 is OK
- 10.21105/astro.2210.13260 is OK

MISSING DOIs

- No DOI given, and none found for title: Healpix.jl: Julia-only port of the HEALPix library
- No DOI given, and none found for title: DUCC
- No DOI given, and none found for title: Euclid preparation: XXVIII. Modelling of the weak ...

INVALID DOIs

- None