[REVIEW]: HealpixMPI.jl: an MPI-parallel implementation of the Healpix tessellation scheme in Julia #6467
Comments
editorialbot
added
Julia
review
TeX
Track: 7 (CSISM)
|
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: For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type: |
|
|
Software report: Commit count by author: |
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
|
License info: 🟡 License found: |
|
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! |
Review checklist for @marcoboniciConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Review checklist for @baxmittensConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
|
Hello @LeeoBianchi is there a way I can confirm the performance claims you have made in this paper? 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)
endJust drop a message if there is anything unclear and I will try to help you. Thank you for undertaking this review process :) Cheers, |
|
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. |
|
I have run the script on 1-4 MPI processes HpixMPI_adj_full_Nside4096_1MPI_32threads.txt HpixMPI_a2m_full_Nside4096_1MPI_32threads.txt HpixMPI_adj_full_Nside4096_2MPI_32threads.txt HpixMPI_a2m_full_Nside4096_2MPI_32threads.txt HpixMPI_adj_full_Nside4096_3MPI_32threads.txt HpixMPI_a2m_full_Nside4096_3MPI_32threads.txt HpixMPI_adj_full_Nside4096_4MPI_32threads.txt HpixMPI_a2m_full_Nside4096_4MPI_32threads.txt |
|
|
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.
Cheers, |
|
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. Greetz, Max |
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.
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, |
|
Did you notice that I commited a pull request for your paper.md? |
|
@LeeoBianchi |
|
@LeeoBianchi I added a couple of comments, opening github issues as described here. |
|
@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. |
By expanding do you mean to provide some more details here, or to add something in the paper? |
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. |
|
Done! archive is now 10.5281/zenodo.11192548 |
|
@editorialbot generate pdf |
|
@LeeoBianchi, thank you. Moving to the next and final stage for publication. Thank you, @marcobonici and @baxmittens, for your time and efforts in reviewing this work for JOSS. I appreciate it. |
|
@editorialbot recommend-accept |
|
|
|
👋 @openjournals/csism-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#5354, then you can now move forward with accepting the submission by compiling again with the command |
editorialbot
added
the
recommend-accept
|
@LeeoBianchi - As track editor, I've proofread the paper and suggested changes in LeeoBianchi/HealpixMPI.jl#10. In addition, something is wrong with the grammar of the second sentence of the summary, and I can't suggest a fix because I'm a little unsure what the intent of the sentence is. Another issue is that SHT sometimes is used for SHTs, and sometimes SHTs is used. I suggest using SHT for one, and SHTs for multiple, consistently through the paper. Please let me know when the PR is merged (or what you disagree with), and when you have addressed the other two points, then we can proceed to publication. |
Hi, thank you for proofreading my paper. I merged your PR as all the changes proposed looked good. I tried to rephrase the sentence you pointed out, I hope it is more clear now. You were right about the SHT/SHTs issue, I think grammarly messed up most of them, I should have fixed that too. I pushed alle the fixes in a new commit, let me know I should fix anything else. Cheers |
|
I've added some comments in your commit |
I should have implemented all the proposed changes. Thank you! |
|
I've added one more small comment on LeeoBianchi/HealpixMPI.jl@dc177d0 - once this is fixed, I think we'll be ready to go. |
Done! |
|
@editorialbot recommend-accept |
|
|
|
👋 @openjournals/csism-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#5358, then you can now move forward with accepting the submission by compiling again with the command |
|
@editorialbot accept |
|
|
Ensure proper citation by uploading a plain text CITATION.cff file to the default branch of your repository. If using GitHub, a Cite this repository menu will appear in the About section, containing both APA and BibTeX formats. When exported to Zotero using a browser plugin, Zotero will automatically create an entry using the information contained in the .cff file. You can copy the contents for your CITATION.cff file here: CITATION.cff
If the repository is not hosted on GitHub, a .cff file can still be uploaded to set your preferred citation. Users will be able to manually copy and paste the citation. |
|
🐘🐘🐘 👉 Toot for this paper 👈 🐘🐘🐘 |
|
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
|
Congratulations to @LeeoBianchi (Leo Alessandro Bianchi) on your publication!! And thanks to @marcobonici and @baxmittens for reviewing, and to @prashjha for editing! |
|
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets: This is how it will look in your documentation: We need your help! The Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
|
Many thanks @danielskatz @prashjha @marcobonici @baxmittens for your time to review my paper! |
Submitting author: @LeeoBianchi (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: @prashjha
Reviewers: @marcobonici, @baxmittens
Archive: 10.5281/zenodo.11192548
Status
Status badge code:
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:
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
The text was updated successfully, but these errors were encountered: