-
-
Notifications
You must be signed in to change notification settings - Fork 36
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]: DORiE: A discontinuous Galerkin solver for soil water flow and passive solute transport based on DUNE #2313
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @gassmoeller, @pratikvn 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 #2313 with the following error: Can't find any papers to compile :-( |
@gassmoeller @pratikvn Thanks again for agreeing to review. Checklists have been generated for each you above, ready for your review :-) |
@whedon generate pdf from branch joss-paper |
|
@whedon commands |
Here are some things you can ask me to do:
|
@whedon check references |
@peanutfun, A very nice library! I have a few comments:
@meg-simula , Would there be a way to check the references through whedon ? I guess the command does not work now because the paper and the .bib file are in a different branch. |
Perhaps I could comment on this. There is indeed some recent initial support for DUNE in spack so the idea is not completely out of the range. I will give it a try this week. Thank you for the suggestion. EDIT: See issue #195 |
@pratikvn, thank you for your quick feedback!
The last author Ole Klein made contributions to both project directions and code. He developed the initial version of the essential Richards DG local operator (see
I have no experience with spack myself but it seems like we could get help on that by the DUNE devs. As maintaining a spack configuration will cause an additional support overhead, I would personally only see it as "nice to have". This actually relates to your second point. With the current installation instructions and Docker support, we think that we cover most of the experience levels of possible users and developers of DORiE: Inexperienced users can simply use a Docker image to run the application, possibly even on a server cluster. Inexperienced developers will likely have a nearly "clean system" as envisaged in our If the installation of DORiE causes problems, I would therefore primarily work on clarifying the instructions in the @SoilRos: Thanks for pitching in! 😊 |
I believe active project direction and contributions to the code are sufficient. I don't think an "Author Contributions" section is necessary. Thanks for clarifying this.
Yes, this was just a suggestion. One of the main reasons I suggest adding a spack package is that HPC users can install it on their cluster themselves (which they do not have root access to and hence cannot use docker) very easily and the fact that dune already has a spack package should simplify the process greatly. The docker installation went smooth for me. But installation by source is definitely more involved. The dune-installer with dune control definitely helps, but I guess the fact that the list of dependencies is quite large makes installation quite lengthy. Nevertheless, the instructions on To make the installation by source slightly easier, I would suggest adding a bootstrapping script. Assuming that all the basic library dependencies ( |
@pratikvn, I'm glad we could resolve the author issue and that you had no trouble building and running DORiE. Thank you for elaborating on the use cases of spack. I realize that missing root privileges is an issue we have not considered much. Supporting such a setup would be a nice addition. At your discretion, however, we would consider the associated issue dorie#195 as non-essential for this review.
Cloning the modules is indeed the worst described part of the installation process. I like your suggestion of using a script for this part (only). Additionally, I propose to add a DUNE buildsystem options file, which would simplify the call to The installation process would then change as follows:
|
Yes, sure. I don't believe that it is essential for this review.
This sounds reasonable to me. The new installation process does look a lot more simpler. Thanks. |
Let me check and get back to you. |
@whedon check references from branch joss-paper |
|
|
@peanutfun, I found no major obstacles to publication, but would like to make two suggestions and have one question, and one comment:
vs
I am aware that there will always be lines that do not follow all best practices so this is not a recommendation to fix this particular instance for this review. Just something to keep in mind for the future development of DORiE in general. I liked this software, keep up the good work! |
@gassmoeller, thanks a lot for your quick review and positive feedback!
Sounds good! We did not do so before because we felt that somebody who can manage development through a mounted Docker container will also manage the manual installation process and end up with a much more capable development environment. Anyway, I'll add this through an MR.
I very much like the Citation File Format and already raised an issue in the DORiE repository, see dorie#199.
DUNE generally supports parallelism via MPI, depending on the actual grid and solver setup. We built DORiE to be run in parallel on a best-effort basis. As we did not want to support both sequential and parallel builds simultaneously, we instruct users to always install OpenMPI, which is technically not required to build DUNE. Therefore, any build of DORiE is capable of running the application in parallel. The same is true for the Docker image. Parallel execution currently is only supported on structured rectangular grids, that is, with the config file parameters In terms of the documentation, the feature is indeed somewhat hidden. Should we advertise it in the Feature Overview and/or mention it in the
This is true, especially for the file you mention. In most of these instances, the naming convention then follows usual code practices of the DUNE-PDELab package, see https://gitlab.dune-project.org/pdelab/dune-pdelab/-/blob/master/dune/pdelab/test/test-transport-ccfv.cc#L231 for an example. The rationale behind some of the names is following the symbols of the numerical problem formulation. For example, the mathematical symbol for the solution (vector) typically is We have a certain interest in keeping the naming convention close to the one used in PDELab because that simplifies communicating code with the PDELab devs. However, you are right about the issue that the code might become difficult to read for people who are less involved. As some of the files also have a (historically grown) bad formatting, I opened dorie#200 to suggest a few changes and discuss the issue with the other DORiE devs. |
I too found no major obstacles for publishing this paper. Some final comments:
Overall, I liked the software and the paper and commend your efforts. |
@pratikvn, thanks again for your comments! Let me answer them one by one...
The idea of the Cook Book section is to have real use-case examples, which will eventually cover all the (more technical) information given in the manual. I'm not entirely sure what you mean by "code and explanations". For the internals of the C++ code, we provide the Doxygen documentation, which is aimed at developers. Here we explain how the internals of DORiE work, which is intended to help modifying the code base or integrating it into another project. The latter is what was done for our latest publication, https://doi.org/10.1002/vzj2.20040, which is also featured in the JOSS paper. For this we extended DORiE with an interface for the data assimilation framework we used. The interface is publicly available in dorie!134. However, we cannot publish the entire code base for the paper because the data assimilation framework has not been published.
Thank you for this suggestion. I personally prefer automation through a git commit hook. Build systems like CMake are designed to construct and modify a build directory tree from a source tree. Modifying the source tree with the build system seems invasive to me. Git, on the other hand, only manages the source tree, so it seems to me like enforcing a format style through it is the "cleaner" approach.
I added more information on the capabilities of DORiE (and their respective caveats) in dorie!209.
I was not aware of this project and its community. I will consider registering DORiE as "xSDK compatible" package.
I added a coverage badge to the DORiE GitLab repository. |
@pratikvn, @gassmoeller, I'm very happy about your feedback and suggestions! To move forward, I want to make sure we are keeping track of all issues you consider crucial for this review. We created a milestone listing all such issues and associated merge requests: https://ts-gitlab.iup.uni-heidelberg.de/dorie/dorie/-/milestones/6. Here's a checklist of your proposals that I understood to be required:
Please check if this list is complete and tell me if there's anything else we need to take care of. Finally, I did some cosmetic updates to the JOSS paper:
I'll compile the latest version now. |
@whedon generate pdf from branch joss-paper |
@whedon set version as 2.0.1 |
I'm sorry human, I don't understand that. You can see what commands I support by typing:
|
@whedon set 2.0.1 as version |
OK. 2.0.1 is the version. |
@whedon set 10.5281/zenodo.3997152 as archive |
OK. 10.5281/zenodo.3997152 is the archive. |
@whedon accept |
|
PDF failed to compile for issue #2313 with the following error: Can't find any papers to compile :-( |
@whedon accept from branch joss-paper |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#1664 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1664, then you can now move forward with accepting the submission by compiling again with the flag
|
@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... |
Congratulations, @peanutfun, your JOSS paper is published! 🚀 Big thanks to our editor: @meg-simula, and the reviewers: @gassmoeller, @pratikvn — we couldn't do this without you 🙏 |
🎉🎉🎉 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:
|
Many thanks to everybody involved! 😊 |
@editorialbot reaccept |
|
🌈 Paper updated! New PDF and metadata files 👉 openjournals/joss-papers#3471 |
@arfon thank you very much! 🙌 |
Submitting author: @peanutfun (Lukas Riedel)
Repository: https://gitlab.dune-project.org/dorie/dorie
Branch with paper.md (empty if default branch):
Version: 2.0.1
Editor: @meg-simula
Reviewer: @gassmoeller, @pratikvn
Archive: 10.5281/zenodo.3997152
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
@gassmoeller & @pratikvn, 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 @meg-simula know.
✨ Please try and complete your review in the next six weeks ✨
Review checklist for @gassmoeller
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @pratikvn
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: