search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
717 stars 38 forks source link

[REVIEW]: MOLE: Mimetic Operators Library Enhanced #3380

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@jcorbino<!--end-author-handle-- (Johnny Corbino) Repository: https://github.com/jcorbino/mole Branch with paper.md (empty if default branch): Version: v3.0.0 Editor: !--editor-->@jedbrown<!--end-editor-- Reviewers: @ketch, @jshipton Archive: Pending

:warning: JOSS reduced service mode :warning:

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

Status badge code:

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

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

@ketch & @jshipton, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jedbrown 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 @ketch

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @jshipton

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ketch, @jshipton it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

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.

:star: Important :star:

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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.cam.2019.06.042 is OK
- 10.1137/S0895479801398025 is OK
- 10.21105/joss.00026 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

Failed to discover a Statement of need section in paper

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.13 s (1293.1 files/s, 78572.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                            52            378            145           3778
MATLAB                          84            654            688           2428
C++                              8            109             90            648
PHP                              1             14             26            289
Markdown                         4             74              0            151
C/C++ Header                     8             43             20            122
CSS                              1             13              0             77
make                             2             21             13             34
TeX                              1              2              0             30
ProGuard                         1              8              5             25
YAML                             1              1              0             18
-------------------------------------------------------------------------------
SUM:                           163           1317            987           7600
-------------------------------------------------------------------------------

Statistical information for the repository 'b2e1665c1b4fb6a8365a819e' was
gathered on 2021/06/18.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Johnny                           1            57              0            1.06
Johnny Corbino                   2           135              1            2.54
Johnny Corbino Delga            35          3003           2162           96.40

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Johnny Corbino Delga       1032           34.4         36.2               10.85
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jedbrown commented 3 years ago

@ketch @jshipton :wave: Welcome to JOSS and thanks for agreeing to review!

The comments from @whedon up top :point_up: outline the review process, which takes place in this thread (possibly with issues filed in the MOLE repository). I'll be watching this thread if you have any questions.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/3380 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We typically aim for reviews to be completed within a month or so. (I know you have constraints and may have limited time until August. If you have initial reactions, feel free to share them early so they can be addressed before more detailed review.) Please let me know if you have further questions. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@jedbrown) if you have any questions/concerns.

whedon commented 3 years ago

:wave: @jshipton, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @ketch, please update us on how your review is going (this is an automated reminder).

jcorbino commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jedbrown commented 3 years ago

:wave: @jshipton @ketch How are your reviews going? Anything issues we can help with or an estimate of when you'll have time?

jshipton commented 3 years ago

Apologies for the slow reply - I've been on leave and am now at a conference. I was in touch with the author (see https://github.com/jcorbino/mole/issues/8) about installation instructions for Macs...

jcorbino commented 3 years ago

Here's a one page document with steps to install MOLE on MacOS. https://www.dropbox.com/s/0u7x9rrqy1onvst/MOLE%20installation%20on%20MacOS.pdf?dl=0 . Basically, for the average Mac user, the way to go is by using Eigen and not SuperLU. The document has also been uploaded to the root directory of the repo.

Kind regards,

jcorbino commented 3 years ago

https://github.com/jcorbino/mole/blob/master/MOLE%20installation%20on%20MacOS.pdf @jshipton just in case.

Kind regards,

ketch commented 2 years ago

Some quick comments:

jcorbino commented 2 years ago

Thanks @ketch for your comments, here's what I did:

Thanks again for your input! I hope I addressed all the comments :)

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

ketch commented 2 years ago

@jedbrown In my view it wouldn't be reasonable to accept this paper without an inclusion of references to existing similar software (e.g. https://doi.org/10.1016/j.cam.2013.12.046) and explanations of how this software compares. Since the authors have explicitly chosen not to include this, even after my request, I can't recommend publication.

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jcorbino commented 2 years ago

@ketch Sorry, I misunderstood what you asked me to do. Please find it in the new version of the draft which I'm going to generate right after this comment.

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jcorbino commented 2 years ago

Hello @jshipton and @ketch,

Did you have a chance to read the last version of the draft? I think I addressed all your comments. @jshipton please refer to https://github.com/jcorbino/mole/blob/master/MOLE%20installation%20on%20MacOS.pdf for instructions on installation on Macs.

Kind regards,

jedbrown commented 2 years ago

Thanks for the updates @jcorbino. Would you consider it appropriate to mention connections with mixed finite element methods (algebraically equivalent in some limited cases, and able to preserve similar properties)?

Can MOLE be used for problems like shallow water with Coriolis or the p-Laplacian? Also, I feel the paper misses an opportunity to demonstrate the benefit of mimetic methods by showing how standard methods may fail to converge on certain problems or do not preserve key properties of the continuum solutions. I know the mimetic literature discusses these things, but you may have readers who are unfamiliar with those examples and solving a 1D Poisson problem in the viewgraph norm is ... underwhelming.

I notice that your users manual says

MOLE is distributed under a "dual-license" model. GPLv3 for academic and nonprofit purposes, and copyright for commercial intentions.

This is not acceptable as written. GPLv3 absolutely can be used commercially (that's Red Hat's business, for example) and you can't imply that it's only GPLv3 for non-commercial purposes. It isn't open source if a company can't sell support for an application built on it, or use in an in-house product. You may advertise that alternate licensing is available if a user isn't satisfied with the terms of GPLv3, though this will have an impact on your contributing guidelines. (Would a valued contributor get paid as part of such licensing? Must they sign a copyright assignment or grant you a different license when submitting their PR so that you have authority to grant a commercial user a different license? Note that inbound=outbound is the de facto assumption in open source contributions, sometimes codified through the likes of a Developers Certificate of Origin or a Contributor License Agreement.)

Less importantly, the OSX installation docs are basically text in a PDF without source, which makes it hard to edit (e.g., for a contributor who streamlines something or needs to make a note for a newer OS version). Those would be easier to read and copy/paste from if converted to markdown (a 2-minute copy/paste job).

ketch commented 2 years ago

I agree with @jedbrown that using Laplace as your main example seems to totally miss the point of mimetic methods.

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jcorbino commented 2 years ago

@jedbrown @ketch thank you for the feedback. I think I've addressed all of them in the latest version of the draft (which was generated right before this comment).

@ketch I would greatly appreciate if you share your list indicating the shortcomings in the writing of the article, I have to admit that your comment caught me by surprise. Lastly, thank you for pointing out that one of the references was missing an author! (fixed)

jcorbino commented 2 years ago

Also in the new draft: "It is important to mention that MOLE's main role is the construction of matrices that represent spatial derivative operators and boundary conditions; other components such as solvers and time steppers are only provided via self-contained examples." I hope this is clear enough.

jcorbino commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jedbrown commented 2 years ago

There's a sizable intersection of mixed FE and mimetic FD methods that are algebraically equivalent. FEEC gives a range of methods that satisfy the discrete compatibility, albeit going to high order in a different way (see "periodic table" of finite elements). As example of broader impact, note that FEEC is the foundation of the UK Met Office's new dynamical core (will be used for weather and climate). I'm not an expert in SBP methods, but I understand they also have some overlap. This is a JOSS paper on one such toolkit: https://joss.theoj.org/papers/10.21105/joss.03454.pdf

I tried running convection_diffusion and get an error

error: interp3: X, Y, Z, and V dimensions must be equal

I'm using Octave, but this isn't the usual kind of error one gets when Octave doesn't implement something needed by a Matlab script.

I don't see how to run tests. There isn't a make test much less continuous integration. I understand that someone can go into the examples and run them, but it's unclear how to determine whether it's correct. I understand that some produce figures that show analytic and numerical solutions, but they only plot the solution and don't do a grid convergence study. I also see that hyperbolic examples use forward Euler or Leapfrog with a time step of exactly 1 CFL on a uniform grid, which isn't discerning (e.g., first order upwind is exact). If you compare SummationByPartsOperators.jl (linked above), you'll see that there's a github action and runtests.jl (the Julia convention) that runs timed tests including comparisons of conventional and SBP operators. All these tests return clear pass/fail so that subjective or expert user knowledge is not needed to interpret the results. As numerical analysts, we should take verification seriously enough to put in our repositories sufficient tooling to test order of accuracy and other discretization properties that we claim to provide.

Honestly, I would drop the phrase "faithful to the physics" -- these discretizations don't preserve positivity for advection, for example. There's good reason for this (Godunov's theorem), but I would just state what is preserved (discrete de Rham complex). I think it's worth writing out the fundamental properties that define mimetic methods rather than outsourcing it to a paywalled paper. It's also important to discuss limitations. I don't know if MOLE would be appropriate for nonlinear spatial discretizations. The elliptic1D example produces a nonsymmetric matrix that is indefinite and has complex eigenvalues -- this might not be appropriate if one wishes to do eigenanalysis. Our documentation should clearly state what is available, what is in-scope but not yet implemented, and what is out of scope. A prospective user shouldn't need to become an expert in MFD to determine whether MOLE will be suitable for their application.

The doc_MATLAB docs work in Chromium, but not in Firefox. Some of the docstrings are wrong. For example, div2DCurv says "Get the determinant of the jacobian and the metrics", but it returns a divergence operator. Am I reading correctly that curl is only supported for uniform grids in 2D? Maybe a table showing operator, dimension, and uniform, non-uniform Cartesian, and curvilinear would be helpful. This could go in the project docs (maybe README) and/or in the paper.

jedbrown commented 2 years ago

Hi, @jcorbino. I see you've pushed some changes to the repository so I'll rebuild the paper. Could you let us know how things are going?

jedbrown commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jedbrown commented 2 years ago

I marked this "paused". @jcorbino Please let us know when you're ready to continue.

arfon commented 2 years ago

@jedbrown – it seems like it's been over 6 months since we last heard from the author here. Might it be time to reject the submission as stale/abandoned?

jedbrown commented 2 years ago

@jcorbino Please let us know within a week if you intend to continue with this submission.

@editorialbot remind @jedbrown in one week

arfon commented 2 years ago

@editorialbot reject

@jedbrown – as this submission seems to be abandoned, I'm going to proceed to reject.

@ketch, @jshipton, @jedbrown – thanks for your efforts here. Sorry this didn't work out.

editorialbot commented 2 years ago

Paper rejected.