-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: GeophysicalFlows.jl: Solvers for geophysical fluid dynamics problems in periodic domains on CPUs & GPUs #3053
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ranocha, @eviatarbach 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/openjournals/joss-reviews) 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:
|
|
PDF failed to compile for issue #3053 with the following error: Can't find any papers to compile :-( |
@ranocha @eviatarbach make sure to accept the invitation to the reviewers group and to have a look at the reviewer guidelines linked to at the top of this review page. The review process will happen in this issue page, so questions to the author or to me can be added as comments here. |
@whedon generate pdf from branch JOSS-paper |
|
Before getting into the rest of the review, I ran into some installation issues on older versions of Julia. On Julia 1.5.3 the installation works fine. On Julia 1.4.1 (less than a year old), the error FourierFlows/GeophysicalFlows.jl#128 occurs upon first import. I understand this was fixed in GeophysicalFlows.jl version 0.8.1, but that version requires Julia 1.5.0, meaning that the package installation is broken on Julia 1.4.1 (and I assume all versions of Julia between 1.4.0 and 1.5.0). I suggest that either a fix is backported or the package be marked as incompatible with these versions. I also tried installing it on Julia 1.3.1, which results in an error about unsatisfiable requirements. This one is perhaps not as important because it gives an explicit error at installation time. However, I think it would be good to determine which versions of Julia this package is compatible with at present (especially versions since Julia 1.0.5, the LTS), and state this in the repository README. |
Thanks @eviatarbach for pointing this out. I’ll try to sort it out and ping you. |
Regarding the error on Julia v1.4, I could fix the issue if I could edit a file from the v0.8.0 release. |
@navidcy you can issue a bugfix release 0.8.5 if necessary. Google should advise you not to edit the content of the release :-) Is it necessary to fix older versions for this issue? |
hi @eviatarbach, regarding the issue with Julia v1.4.2, I've pushed a bugfix release v0.8.5 which should be fine.
and
|
@eviatarbach, regarding supporting Julia's longterm release v1.0.5, we can't be using CUDA.jl (since even v0.1.0 requires Julia v1.3). Unfortunately, the latest GeophysicalFlows.jl version that is supported on Julia v1.0.5 is only v0.5.1. |
I have added remark regarding version compatibilities with Julia's releases in the readme; see |
@navidcy typically a JOSS reviews considers a single version of the software. The version listed upon submission is v0.11.3, may I ask what is the relevance of updating earlier versions? |
@navidcy Thank you for addressing this issue! It works now on Julia 1.4 for me as well. @pdebuyl I brought up this issue. I thought that this would be important to address since the installation was failing on even quite recent versions of Julia. I apologize if this was outside the scope of the review. |
@pdebuyl, as @eviatarbach explained I was trying to address their remark above. I felt that adding a note in the readme was indeed very appropriate. |
@eviatarbach this was not out of scope, no problem. I did not get why there was a discussion of older versions of the code but it all makes sense. + There is now a bugfix release and extra information in the README, which is good. |
Thanks for creating this nice package (and the basic functionality in FourierFlows.jl), @navidcy et al. Please find below some comments and suggestions.
|
Thank you @ranocha for the review. The statement of need is supposed to be checked on submission, probably it wasn't because of the non "master" branch for the paper. |
@ranocha, I don't really see how I should reformat the file in view of your remark: "You should consider reformatting the license file LICENSE.md such that GitHub recognizes the license type automatically, cf. https://opensource.org/licenses/MIT" Could you elaborate a bit perhaps? |
|
PDF failed to compile for issue #3053 with the following error:
|
Thanks @ranocha and @eviatarbach for the review! |
@whedon accept from branch JOSS-paper |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#2247 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#2247, then you can now move forward with accepting the submission by compiling again with the flag
|
@pdebuyl I just went through the proofs. Looks good -- I just rephrased a the sentence to reflect that Pearson et al. 2021 is part of what "has been done" and not part of what's "Currently, Can you update the pdf to reflect the latest version? |
Hi @navidcy I'll let the handling associate editor-in-chief take the update into account. They will take the review process further in this review issue. |
@whedon accept from branch JOSS-paper |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#2248 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#2248, then you can now move forward with accepting the submission by compiling again with the flag
|
thanks @arfon, looks good! |
@whedon accept deposit=true from branch JOSS-paper |
|
🐦🐦🐦 👉 Tweet 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... |
@ranocha, @eviatarbach - many thanks for your reviews here and to @pdebuyl for editing this submission. JOSS relies upon the volunteer efforts of people like you, and we couldn't do this without you ✨ @navidcy - your paper is now accepted into JOSS ⚡🚀💥 |
🎉🎉🎉 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! 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:
|
Submitting author: @navidcy (Navid C. Constantinou)
Repository: https://github.com/FourierFlows/GeophysicalFlows.jl
Version: v0.12.1
Editor: @pdebuyl
Reviewer: @ranocha, @eviatarbach
Archive: 10.5281/zenodo.4695260
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.
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
@ranocha & @eviatarbach, 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 @pdebuyl 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 @ranocha
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @eviatarbach
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: