Skip to content
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]: Ocellaris: an exactly incompressible DG FEM solver for free surface flows #1239

Closed
whedon opened this issue Feb 7, 2019 · 64 comments
Closed

Comments

@whedon
Copy link
Collaborator

@whedon whedon commented Feb 7, 2019

Submitting author: @TormodLandet (Tormod Landet)
Repository: https://bitbucket.org/ocellarisproject/ocellaris
Version: v2019.0.2
Editor: @labarba
Reviewer: @inducer, @piyueh
Archive: 10.5281/zenodo.2601988

Status

status

Status badge code:

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

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) 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

@inducer & @piyueh, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @labarba know.

Please try and complete your review in the next two weeks

Review checklist for @inducer

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: v2019.0.2
  • Authorship: Has the submitting author (@TormodLandet) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @piyueh

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: v2019.0.2
  • Authorship: Has the submitting author (@TormodLandet) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon
Copy link
Collaborator Author

@whedon whedon commented Feb 7, 2019

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @inducer, it looks like you're currently assigned as the reviewer for this paper 🎉.

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:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands
@whedon
Copy link
Collaborator Author

@whedon whedon commented Feb 7, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Feb 7, 2019

@labarba
Copy link
Member

@labarba labarba commented Feb 7, 2019

👋 @inducer, @piyueh
Thank you for agreeing to review for JOSS: we couldn't do this without you 🙏

This is where the action happens. You both have a reviewer checklist at the top of this thread. Work your way through that, and feel free to ask questions here and to open issues in the submission repository as needed.

The JOSS review process

@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 9, 2019

@whedon generate pdf

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 9, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 9, 2019

@piyueh
Copy link

@piyueh piyueh commented Mar 10, 2019

@labarba @TormodLandet Regarding the question "Version: Does the release version given match the GitHub release (2019.0.1)?", should I only review this specific version? Or should I review the master branch? It appears there are some new commits in the repository after the commit that is tagged with 2019.0.1.

@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 10, 2019

I have worked on updating the documentation since the release. There should not be any code changes as far as I remember. I can release 2019.0.2 with the current changes if that is preferable. The only change since 2019.0.1 is extending and moving the documentation to www.ocellaris.org and working on the JOSS paper.

@labarba
Copy link
Member

@labarba labarba commented Mar 10, 2019

As the review proceeds, the author will make changes, so commits will accrue. But your review should be on the master branch, unless the author requests that we work on a separate branch for the whole review. What we don't want, though, is to confuse reviewers with more than one branch active in review.

@piyueh
Copy link

@piyueh piyueh commented Mar 10, 2019

OK, I'll review the master branch

@inducer
Copy link

@inducer inducer commented Mar 14, 2019

Here's a marked-up copy of the paper. I've raised a number of concerns.

On the code end (for now):

Other things:

  • Also noticed that two references are missing DOIs.
  • I couldn't find specific community guidelines (see review checklist) in the docs.
@inducer
Copy link

@inducer inducer commented Mar 14, 2019

Also noticed that two references are missing DOIs.

@labarba
Copy link
Member

@labarba labarba commented Mar 16, 2019

Check out the powers of our bot…

@labarba
Copy link
Member

@labarba labarba commented Mar 16, 2019

@whedon check references

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 16, 2019

Attempting to check references...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 16, 2019


OK DOIs

- 10.17226/5870 is OK
- 10.1007/s10915-009-9268-2 is OK
- 10.1007/s10915-011-9501-7 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1007/978-3-642-23099-8 is OK

MISSING DOIs

- https://doi.org/10.1177/1040638717729726 may be missing for title: Ocellaris source code
- https://doi.org/10.7717/peerj.2949/fig-4 may be missing for title: The Ocellaris project web page, user guide and blog
- https://doi.org/10.2172/963444 may be missing for title: Slope limiting the velocity field in a discontinuous Galerkin divergence free two-phase flow solver
- https://doi.org/10.1063/1.168744 may be missing for title: A tensorial approach to computational continuum mechanics using object-oriented techniques
- https://doi.org/10.1016/s0021-9991(03)00298-5 may be missing for title: Gerris: a tree-based adaptive solver for the incompressible Euler equations in complex geometries
- https://doi.org/10.2172/1483828 may be missing for title: PETSc: Portable, Extensible Toolkit for Scientific Computation
- https://doi.org/10.17504/protocols.io.wv2fe8e may be missing for title: meshio
- https://doi.org/10.1109/hpcmp-ugc.2007.30 may be missing for title: The XDMF format
- https://doi.org/10.1063/1.4937536 may be missing for title: The HDF5 format

INVALID DOIs

- None
@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 20, 2019

Thanks for the the review and comments on the paper, @inducer! I have updated the Markdown document and pushed the changes to master.

@whedon generate pdf

Below follows answers/clarifications to some comments

Discussion of finite volume methods

I extended and clarified this section (of "competing" methods). The intent is to show the commonly used methods (piecewise constant, first or second order, finite volume + volume of fluid) and contrast these with DG FEM (the method used in Ocellaris). The lack of Gibbs oscillations in FVM (for low order reconstructions only, of course!) is a plus, but the lack of high order without large reconstruction stencils is a negative for unstructred meshes. I do not claim that a DG FEM method is better, just that it is a fully working alternative, and many people are most familiar with the FVM methods, so comparing to these makes sense to me.

DG FEM performance

As the cited papers show, DG FEM can be faster than continuous FEM when the approximation order increases. Ocellaris can in principle go up to very high order, but this currently only works for single-phase simulations. The C++ implementation of slope limiters for cubic or higher order polynomials is missing, so in practice only linear or quadratic elements can currently be used for two-phase flow simulations. In this range DG FEM does probably not have any computational advantages over CG FEM, so I am not claming that the method is currently very computationally efficient compared to FVM or CG FEM. What I do believe is that this direction of development (higher order DG FEM) may have advantages for two-phase flow simulations despite the greatly increased number of unknowns in the DG basis functions. Some more pieces must fall into place (VOF/level set methods targeted at advection by a higher order DG FEM velocity field, and better preconditioning for the variable density equation with jump in density field are probably the two most important) before Ocellaris can possibly be selected based on a purely computational performance, but I hope Ocellaris can serve as a testbed for some of these advances in the future.

Higher order geometry

Ocellaris supports higher order geometry, but I have not used this much personally, and I do not test it on the CI system, so I'm not sure I want to advertise it right now.

YAML vs Python as configuration

Using Python as a configuration and input file format has obvious benefits such as a very high degree of flexibility. I have, however, worked for some years in a team that had a core tool that used Python scripts as input files. Such files are often "throwaway" code and are given less care and documentation than "real" code. Given the posibilities, many people will construct very clever and efficient workflows, which are extremely hard for others to inspect. This is a problem for QA, sharing, and for training new users. Ocellaris can be used this way, the Simulation.input object can be treated as a nested dictionary and can be created entirely from code. However, I strongly prefer less flexible input files by default. This makes all files follow somewhat of the same pattern, and makes it easier to catch mistakes and learn from simulation input files written by other people. I try to introduce less flexible INI/YAML input file formats in all my projects due to my past experience. It is my goal that 95% of simulations can be configured in a compact and readable manner without introducing code into the configuration, but I do recognize the need to "drop down" into code, and the input file format allows for this. I hope it will not be used by every simulation, and that the main rigid input file layout will make it easy to mentally and programmatically "diff" two simulations, find problems, and understand the intention of the writer.

PETSc bibtex generator (using the "-citations" option)

Done

DOIs

Every reference now has a DOI or URL

@whedon check references

@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 20, 2019

@whedon generate pdf

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 20, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 20, 2019

@inducer
Copy link

@inducer inducer commented Mar 20, 2019

Vada, & Braathen

  • extraneous comma
@inducer
Copy link

@inducer inducer commented Mar 20, 2019

The paper and documentation look good to me now, and the software appears to work as advertised. Once community guidelines are added, I recommend accepting the paper. As for my review checklist, since I reviewed a recent git version (762876f), I recommend rolling a new release, so that I can check off the version-related item.

@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 21, 2019

Thanks both of you for your useful comments! I updated the community guidelines with information about how to run the tests, fixed the installation instruction per the review comments and released a new version, 2019.0.2 on both PyPi and the Singularity Library.

With regards to the extraneous commas in the references, they show up in multiple places in the reference list and seem to be related to the LaTex style used by the automated Markdown->LaTeX->PDF toolchain

@inducer
Copy link

@inducer inducer commented Mar 21, 2019

With regards to the extraneous commas in the references, they show up in multiple places in the reference list and seem to be related to the LaTex style used by the automated Markdown->LaTeX->PDF toolchain

OK, not a deal breaker. Just figured I would point it out.

Thanks both of you for your useful comments! I updated the community guidelines with information about how to run the tests

On that page, I recommend

  • adding a heading "How to report a bug" with a direct link to the issue tracker.
  • adding a heading "How to contribute a patch" with instructions on how to submit the patch. (particularly if someone doesn't already have commit rights)
  • adding a heading "How to get tech support" describing how one might accomplish that.
@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 21, 2019

The development guidelines page was getting long, and was dominated by How to contribute a patch, so I made a new community guidelines page, more prominently featured in the documentation with your three suggested headings. I like how it is now quite clear how to engage, thank you! The new page can be seen here: https://www.ocellaris.org/community_guidelines.html

The development guidelines now has a section on how to merge code, also when you do not have write access to the repo

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 23, 2019

Check final proof 👉 openjournals/joss-papers#574

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#574, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 23, 2019


OK DOIs

- 10.1063/1.168744 is OK
- 10.1016/S0021-9991(03)00298-5 is OK
- 10.1016/j.wavemoti.2019.01.007 is OK
- 10.1017/CBO9780511546068 is OK
- 10.17226/5870 is OK
- 10.1016/0021-9991(81)90145-5 is OK
- 10.2514/6.1979-1469 is OK
- 10.1007/s10915-009-9268-2 is OK
- 10.1007/s10915-011-9501-7 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1007/978-3-642-23099-8 is OK
- 10.1007/978-1-4612-1986-6_8 is OK
- 10.1016/j.advwatres.2011.04.013 is OK
- 10.1145/992200.992206 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@labarba
Copy link
Member

@labarba labarba commented Mar 23, 2019

@whedon set v2019.0.2 as version

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 23, 2019

OK. v2019.0.2 is the version.

@labarba
Copy link
Member

@labarba labarba commented Mar 23, 2019

@TormodLandet — We're ready to publish your paper. Please give the proof one last check and confirm here that you approve the proof (if you feel like it, you could hyphenate your compound adjectives in several places).

@piyueh
Copy link

@piyueh piyueh commented Mar 23, 2019

You've left one item of the review list unchecked. Are you ready to check that off?

Done.

@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 27, 2019

@whedon generate pdf

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 27, 2019

@labarba --- I have done a final proofread of the paper and tried to hyphenate compound adjectives as well as reduce the number of stacked modifiers. One hyphen was also introduced in the title of the paper

@labarba
Copy link
Member

@labarba labarba commented Mar 27, 2019

@whedon accept

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

Attempting dry run of processing paper acceptance...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019


OK DOIs

- 10.1063/1.168744 is OK
- 10.1016/S0021-9991(03)00298-5 is OK
- 10.1016/j.wavemoti.2019.01.007 is OK
- 10.1017/CBO9780511546068 is OK
- 10.17226/5870 is OK
- 10.1016/0021-9991(81)90145-5 is OK
- 10.2514/6.1979-1469 is OK
- 10.1007/s10915-009-9268-2 is OK
- 10.1007/s10915-011-9501-7 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1007/978-3-642-23099-8 is OK
- 10.1007/978-1-4612-1986-6_8 is OK
- 10.1016/j.advwatres.2011.04.013 is OK
- 10.1145/992200.992206 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

Check final proof 👉 openjournals/joss-papers#582

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#582, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
@labarba
Copy link
Member

@labarba labarba commented Mar 27, 2019

The paper has 21 references, but the XML file for the Crossref deposit lists 22 citations. I think this will cause a false citation to be counted by Crossref. Could you delete the entry from you r.bib file that is not being cited in the paper?

@TormodLandet
Copy link

@TormodLandet TormodLandet commented Mar 27, 2019

I removed the unused reference

@labarba
Copy link
Member

@labarba labarba commented Mar 27, 2019

@whedon accept

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

Attempting dry run of processing paper acceptance...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

Check final proof 👉 openjournals/joss-papers#583

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#583, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019


OK DOIs

- 10.1063/1.168744 is OK
- 10.1016/S0021-9991(03)00298-5 is OK
- 10.1016/j.wavemoti.2019.01.007 is OK
- 10.1017/CBO9780511546068 is OK
- 10.17226/5870 is OK
- 10.1016/0021-9991(81)90145-5 is OK
- 10.2514/6.1979-1469 is OK
- 10.1007/s10915-009-9268-2 is OK
- 10.1007/s10915-011-9501-7 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1007/978-3-642-23099-8 is OK
- 10.1007/978-1-4612-1986-6_8 is OK
- 10.1016/j.advwatres.2011.04.013 is OK
- 10.1145/992200.992206 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@labarba
Copy link
Member

@labarba labarba commented Mar 27, 2019

@whedon accept deposit=true

@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

Doing it live! Attempting automated processing of paper acceptance...
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 openjournals/joss-papers#584
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01239
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

@labarba
Copy link
Member

@labarba labarba commented Mar 27, 2019

Congratulations, @TormodLandet, your JOSS paper is published!

Big thanks to the reviewers, @inducer and @piyueh — we couldn't do this without you! 🙏

@labarba labarba closed this Mar 27, 2019
@whedon
Copy link
Collaborator Author

@whedon whedon commented Mar 27, 2019

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.01239/status.svg)](https://doi.org/10.21105/joss.01239)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01239">
  <img src="http://joss.theoj.org/papers/10.21105/joss.01239/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.01239/status.svg
   :target: https://doi.org/10.21105/joss.01239

This is how it will look in your documentation:

DOI

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants