-
Notifications
You must be signed in to change notification settings - Fork 1
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]: Julia for HPC: In Situ data Analysis with Julia for Climate Simulations at Large Scale #134
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @simonbyrne, @williamfgc it looks like you're currently assigned to review this paper 🎉. 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. ⭐ Important ⭐ 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:
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:
|
Failed to discover a |
Wordcount for |
|
|
@luraess the links to the conflict of interest policy and code of conduct don't work. |
@simonbyrne thanks for flagging, while we work on a fix, you can access those here https://juliacon.github.io/proceedings-guide/reviewer/#review_guidelines_and_process |
👋 @simonbyrne, please update us on how your review is going (this is an automated reminder). |
👋 @williamfgc, please update us on how your review is going (this is an automated reminder). |
@simonbyrne @williamfgc Human asking about the status of the revision? Ideally, we can try to get this done in the coming weeks (2 ideally). Thanks so much for your contribution! |
I'm on it, will update later this week. |
Same here, I did a quick pass, but I need to go through it more carefully. |
@simonbyrne @williamfgc also let me know if you still have issue checking tick-boxes. |
This was an interesting application of embedding Julia inside another program for in situ analysis. Although I am not able to assess the scientific validity of the analysis (which seems incomplete), from a technical perspective, this is an important problem: as simulations reach higher resolutions, it becomes more expensive and difficult to store all the data for post-processing analysis, thus it is helpful if such analysis can be done while the simulation is running ("in situ"). Overall, I thought the paper was reasonable, but the exposition could be improved in a few places. There is no linked code, (other than the TributaryPCA.jl package, and this doesn't meet most of the above criteria, such as documentation), so I can't really check off those relevant boxes. While the code may not strictly be required, it is a shame as I think this could be of interest to others who are attempting something similar. I would encourage the authors to make those available if possible. Specific comments3.3 C Interface
This is not what the C code in code 2 is doing: as described in https://docs.julialang.org/en/v1/manual/embedding/#Memory-Management, the 3.5 Noninvasive CompilationSome more details could be useful here: how did you load the Julia code? (e.g. did you make use of a precompiled system image?) How did you switch between (or switch off) the in situ analysis codes? Again, having the actual code available could make this easier. 4.1 Sudden Stratospheric Warming (SSW)I'm not sure Algorithm 1 is correct: shouldn't it reset the There isn't much detail on the actual GEV fitting, and I found it difficult to understand. Is it fitting a different GEV distribution at every point? What is the prior distribution used for the GEV parameters? Some more details on this would be helpful (as well as linking to the code). 4.2 TributaryPCAWhile I understand the problem it is trying to solve, I found it difficult to understand algorithm, and the Code 4 block is somewhat unclear:
(I don't have access to the TributaryPCA paper, so couldn't look it up there either). I would suggest either having a mathematical exposition of the algorithm, and/or making the code a bit more readable. Is the data centered (i.e. do you subtract the mean)? The lack of red in Fig 3, PC1 suggests that it isn't, but that isn't clear. If you aren't then the interpretation changes slightly. 5.2 In Situ SSW Detection and Characterization ResultsI don't quite understand this: it doesn't actually analyse the SSW case? It just seems to be an illustration of how one might do an analysis. 5.3 PCA ResultsThe details are a bit lacking. Was the data collected every time step? Or is it based on daily snapshots or averages? How long was the simulation? I disagree with the interpretation of figure 3: you can't really assign "warmer vs colder" to principal components (the sign of eigenvectors doesn't matter). My interpretation of the figures is that:
While it's useful that you can capture this, it's not particularly scientifically interesting. 5.4 PerformanceThe SSW seems to have negligible overhead (which is not surprising since it is evaluated infrequently), so there is not much to say about the analysis. It could be helpful if you could add the number of MPI ranks to Table 1? I'd be interested to know more about the performance bottlenecks of the PCA code? Which parts were the most expensive? Did the Julia garbage collector cause problems with scaling? Minor comments
|
Review: Julia for HPC In Situ Data Analysis with Julia for Climate Simulations at Large Scale The work present the implementation of an “in-situ” framework with components written in Julia coupled with the well-known E3SM framework model. Two main post-processing tasks are evaluated: Sudden Stratospheric Warming (SSW) and principal component analysis (PCA). SSW is a smaller component while PCA is more computationally intensive due to the required Cholesky decomposition execution. I must admit I am not a doing expert in E3SM, but I can appreciate the computational effort in plugging Julia into an existing framework for a novel application. Computational experiments show that in-situ provides reasonable overheads for a larger number of processing elements (PEs) usage per node. Overall, it’s a straight-forward application of how Julia can be added to an existing framework and add value for post-processing tasks. Major points:
Minor points:
|
Thanks for handing in your revision @simonbyrne @williamfgc . IMPORTANT Please go through the tick-box list and check all points that are addressed leaving the remaining as "to-dos". All tick-boxes will have to be checked upon publication. |
@ltang85 now that the 2 reviewers gave you feedback, please address their comments as soon as possible. Also, please take a look at #134 (comment) as you'd need to fix those references as well. |
@ltang85 just checking in about the status of the revisions, given that you've got feedback since one month ago. Please try to address the issues raised by the reviewers in the coming weeks, and do not hesitate to reach out if there is anything we can help you with. Thanks. |
@luraess I am still working on it and should be able to address all the comments in 2 weeks. Thanks. |
Thanks - take your time. Was just checking in about progress. |
@ltang85 seems there was no activity update for a while here. Can we get the process to a final stage in the coming week? |
@luraess sorry for the delay, I will try to finish it by next week. |
|
👋 @JuliaCon/jcon-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 JuliaCon/proceedings-papers#94, then you can now move forward with accepting the submission by compiling again with the command |
@ltang85 can you review the final proof? |
@luraess just proof read it and made a few minor changes and it should be ready, can you help regenerate it? Thanks. |
@editorialbot recommend-accept |
|
|
👋 @JuliaCon/jcon-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 JuliaCon/proceedings-papers#95, then you can now move forward with accepting the submission by compiling again with the command |
@ltang85 can you have final check. Then we should be good to go |
@luraess I generated a local pdf by using this repo and everything looks great, but this proofread version still has the 2nd and 4th authors with their old affiliations before some changes, please see the attached local version |
@ltang85 yes - there is a way around. I need to recall - keep you posted. |
@luraess thanks |
@ltang85 I see e.g. in this commit you edited directly the |
Thanks @lucaferranti for your input. @ltang85 please update the corresponding yml file and check with |
@editorialbot generate pdf |
@lucaferranti thanks for the fix and it works now. @luraess everything looks great now and it is ready to go |
@editorialbot recommend-accept |
|
|
👋 @JuliaCon/jcon-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 JuliaCon/proceedings-papers#96, then you can now move forward with accepting the submission by compiling again with the command |
@editorialbot accept |
|
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JCON! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
@ltang85 congratulation for having your paper published in the JuliaCon Proceedings! Thanks for your cooperation during the entire revision and publication process. |
I am closing the issue now. If anything, please ping me. Thanks all once more. |
🎉🎉🎉 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! JuliaCon Proceedings 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:
|
Submitting author: @ltang85 (Li Tang)
Repository: https://github.com/ltang85/In-Situ-data-Analysis-with-Julia-for-Climate-Simulations-at-Large-Scale
Branch with paper.md (empty if default branch):
Version: v1.0
Editor: @luraess
Reviewers: @simonbyrne, @williamfgc
Archive: 10.5281/zenodo.12104097
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
@simonbyrne & @williamfgc, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @luraess 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 @simonbyrne
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Paper format
paper.tex
file include a list of authors with their affiliations?Content
Review checklist for @williamfgc
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Paper format
paper.tex
file include a list of authors with their affiliations?Content
The text was updated successfully, but these errors were encountered: