[REVIEW]: ngsxfem: Add-on to NGSolve for geometrically unfitted finite element discretizations

whedon commented 3 years ago

Submitting author: @schruste (Christoph Lehrenfeld) Repository: https://github.com/ngsxfem/ngsxfem Version: joss Editor: @meg-simula Reviewer: @mscroggs, @mikaem Archive: 10.5281/zenodo.5081124

: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 badge code:

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

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

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

Make sure you're logged in to your GitHub account
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 @meg-simula 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 @mscroggs

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@schruste) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @mikaem

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@schruste) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[ ] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[ ] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @mscroggs, @mikaem, @mikaem 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:

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

watching

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

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.58 s (238.7 files/s, 46246.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             38           1931           1057           9911
Python                          47           1795           2075           4859
C/C++ Header                    32            671            297           2821
CMake                           12             87              9            361
TeX                              1             39              1            343
Markdown                         4            102              0            302
YAML                             3             14              0            213
Bourne Shell                     1              5              6             14
Dockerfile                       1              7              2             11
-------------------------------------------------------------------------------
SUM:                           139           4651           3447          18835
-------------------------------------------------------------------------------

Statistical information for the repository '08040c6e9b48146c3b1f2118' was
gathered on 2021/05/04.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Christoph Lehrenfeld           974         53534          58645           60.14
Christoph Winterstei             2           339             24            0.19
Christopher Lackner              2             7              7            0.01
Fabian Heimann                 466         14595           6387           11.25
Henry Maximilian von            64          2461           1586            2.17
Henry v. Wahl                   97          3712           1354            2.72
Henry von Wahl                   4           153              1            0.08
Janosch Preuß                  100          6502           2186            4.66
Matthias Hochsteger              6           193            157            0.19
Philip Lederer                   5           636             13            0.35
Thomas Ludescher                17          3637            341            2.13
Tobias Jawecki                   1             3              3            0.00
Yimin Lou                        6           209             13            0.12
henryvonwahl                     2           333              0            0.18
hvonwah                          1             6              4            0.01
lehrenfeld                       1            93             32            0.07
mw                               2            77              1            0.04
root                             1           316             79            0.21
schruste@pummelzacke           190         22159           6078           15.14
tobias                           7           634             20            0.35
yimin.lou                        1             1              1            0.00

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
Christoph Lehrenfeld      16376           30.6         37.6                8.36
Christopher Lackner           6           85.7         38.1                0.00
Fabian Heimann             4211           28.9         27.0                9.45
Henry Maximilian von       2960          120.3          7.0               14.86
Matthias Hochsteger          23           11.9         29.0               39.13
Thomas Ludescher            872           24.0         24.7               16.51
henryvonwahl                270           81.1         15.6                5.93
janosch.preuss              623          100.0         45.9               11.08
root                         76           24.1         52.5                2.63

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:

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/5.0037971 is OK
- 10.5281/zenodo.3989604 is OK
- 10.5281/zenodo.3647571 is OK
- 10.1051/m2an/2020044 is OK
- 10.1016/j.camwa.2019.08.003 is OK
- 10.1002/pamm.201610003 is OK
- 10.1016/j.cma.2015.12.005 is OK
- 10.1007/978-3-319-71431-8_3 is OK
- 10.1093/imanum/drx041 is OK
- 10.1007/978-3-319-96415-7_16 is OK
- 10.1051/m2an/2018068 is OK
- 10.1515/jnma-2017-0109 is OK
- 10.18154/RWTH-2020-07305 is OK
- 10.1137/16m1102203 is OK
- 10.1007/s00211-016-0801-6 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1007/978-3-642-28589-9_7 is OK
- 10.11588/ans.2016.100.26526 is OK

MISSING DOIs

- 10.1093/imanum/drz062 may be a valid DOI for title: Trace Finite Element Methods for Surface Vector-Laplace Equations
- 10.7712/100016.1820.4573 may be a valid DOI for title: Removing the stabilization parameter in fitted and unfitted symmetric Nitsche formulations

INVALID DOIs

- None

meg-simula commented 3 years ago

:wave: @mscroggs @mikaem Thanks again for agreeing to review. Reviewer checklists have been generated for you above, feel free to get started whenever is convenient for you in the nearest few weeks. I'll check back in regularly, and do not hesitate to ask if you have questions.

mikaem commented 3 years ago

Hi @schruste,

I have had a first look and this appears to be a great piece of software, and a paper satisfying most of the JOSS check-points:-)

I have just a few issues. Mainly, there is no Statement of need section in the paper, and the paper is quite long (2100 words) compared to the guidelines (250-1000 words). I also think there is too much detail in the paper. For example, figures 2-5 are quite detailed and contain symbols and notation that is not explained in the figure caption nor in the text.

Regarding installation, I do not have NGsolve installed locally so I relied on the binder image, that works nicely:-) I also tried the docker installation, but apparently there is no browser included, so I do not know how to run the jupyter notebooks in the container? I may be stupid here, but the guidelines simply says open a browser and past in the URL that you obtain in the terminal, and to my best understanding this needs to be done inside the container and the container does not seem to have a browser?

schruste commented 3 years ago

Hi @mikaem ,

Thank you for the feedback. It seems we actually overlooked the guideline on the length at first, but repaired this now. We moved the more detailed parts to separate markdown files in doc (which shouldn't be directly relevant for the review anymore).

As for the docker question. We added a sentence in the INSTALLATION.md to emphasize that it is really the browser of the host system from which you spawn the docker image (not a browser in the docker!) that can be used (thus the port forwarding in the spawning command). The docker image itself does not need to have a browser.

The updated changes are in the master branch (which is not the default branch) for now.

Best, Christoph

schruste 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:

schruste commented 3 years ago

@whedon generate pdf from branch master

whedon commented 3 years ago

Attempting PDF compilation from custom branch master. Reticulating splines etc...

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:

whedon commented 3 years ago

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

whedon commented 3 years ago

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

whedon commented 3 years ago

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

mscroggs commented 3 years ago

Hi @schruste,

This is a nice piece of software, and a good write up. I successfully ran some demos in the docker image, and also successfully installed xfem using pip (after successfully installing NGSolve using apt).

I left two boxes unchecked as I have a couple of minor changes to suggest:

(Installation) INSTALLATION.md points to docker image ngsxfem/ngsxfem, but the readme points to docker image schruste/ngsxfem. I think the first link is correct and the readme should be updated?
(References) I think the following DOIs are valid for the two references currently without DOIs or links:
- Badia, Martin, Principe: https://dx.doi.org/10.1007/s11831-017-9244-1
- Bastian, Heimann, Marnach: https://dx.doi.org/10338.dmlcz/140745

schruste commented 3 years ago

Hi @mscroggs,

Thank you for the nice feedback and for the reports. We added the DOIs and fixed the README.md!

schruste commented 3 years ago

@whedon generate pdf from branch master

whedon commented 3 years ago

Attempting PDF compilation from custom branch master. Reticulating splines etc...

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:

mscroggs commented 3 years ago

Thanks, My checklist items are now all ticked and I'm happy with the paper as it now stands

meg-simula commented 3 years ago

Excellent, thanks @mscroggs and for the prompt handling of comments @schruste.

@mikaem: How about your remaining items, would you give us an update on these?

mikaem commented 3 years ago

I had a final reading of the article and I saw two more minor issues. There are some periods missing in the last paragraph before acknowledgements, and I seem to be mentioned twice in the left column under reviewers for some reason?

schruste commented 3 years ago

Hi @mikaem ,

Thanks for the hint with the period (we fixed it). I don't know why you are mentioned twice, but as far as I see this is not related to our content but because you are listed twice in this issue (Reviewer: @mscroggs, @mikaem, @mikaem).

schruste commented 3 years ago

@whedon generate pdf from branch master

whedon commented 3 years ago

Attempting PDF compilation from custom branch master. Reticulating splines etc...

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:

meg-simula commented 3 years ago

Great, we'll resolve the duplicate reviewer listing from this side. @mikaem Are you also happy with this submission now?

mikaem commented 3 years ago

I'm fine @meg-simula

meg-simula commented 3 years ago

Great, thanks all! I'll follow up from here in the course of the week.

schruste commented 3 years ago

Hi @meg-simula . I'd like to kindly ask for an update. Is there anything that we should do?

meg-simula commented 3 years ago

Hi @schruste, thanks for the reminder - this is just waiting for me to get a chance to look though it. No need to do anything from your side at the moment, and I will get to it shortly.

schruste commented 3 years ago

Hi @meg-simula . May I kindly ask for update?

meg-simula commented 3 years ago

I'll look at tonight, thanks for the patience.

meg-simula commented 3 years ago

@whedon generate pdf from branch master

whedon commented 3 years ago

Attempting PDF compilation from custom branch master. Reticulating splines etc...

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:

meg-simula commented 3 years ago

@whedon check references

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/5.0037971 is OK
- 10.5281/zenodo.3989604 is OK
- 10.5281/zenodo.3647571 is OK
- 10.1051/m2an/2020044 is OK
- 10.1016/j.camwa.2019.08.003 is OK
- 10.1002/pamm.201610003 is OK
- 10.1016/j.cma.2015.12.005 is OK
- 10.1007/978-3-319-71431-8_3 is OK
- 10.1093/imanum/drx041 is OK
- 10.1007/978-3-319-96415-7_16 is OK
- 10.1051/m2an/2018068 is OK
- 10.1515/jnma-2017-0109 is OK
- 10.18154/RWTH-2020-07305 is OK
- 10.1137/16m1102203 is OK
- 10.1007/s00211-016-0801-6 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1007/978-3-642-28589-9_7 is OK
- 10.11588/ans.2016.100.26526 is OK

MISSING DOIs

- 10.1093/imanum/drz062 may be a valid DOI for title: Trace Finite Element Methods for Surface Vector-Laplace Equations
- 10.7712/100016.1820.4573 may be a valid DOI for title: Removing the stabilization parameter in fitted and unfitted symmetric Nitsche formulations

INVALID DOIs

- None

meg-simula commented 3 years ago

@whedon check references from branch master

whedon commented 3 years ago

Attempting to check references... from custom branch master

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/5.0037971 is OK
- 10.5281/zenodo.3989604 is OK
- 10.1093/imanum/drab044 is OK
- 10.5281/zenodo.3647571 is OK
- 10.1051/m2an/2020044 is OK
- 10.1093/imanum/drz062 is OK
- 10.1016/j.camwa.2019.08.003 is OK
- 10.1002/pamm.201610003 is OK
- 10.1016/j.cma.2015.12.005 is OK
- 10.1007/978-3-319-71431-8_3 is OK
- 10.1093/imanum/drx041 is OK
- 10.1007/978-3-319-96415-7_16 is OK
- 10.1051/m2an/2018068 is OK
- 10.1515/jnma-2017-0109 is OK
- 10.18154/RWTH-2020-07305 is OK
- 10.7712/100016.1820.4573 is OK
- 10.1137/16m1102203 is OK
- 10.1007/s00211-016-0801-6 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1007/978-3-642-28589-9_7 is OK
- 10.11588/ans.2016.100.26526 is OK
- 10338.dmlcz/140745 is OK
- 10.1007/s11831-017-9244-1 is OK

MISSING DOIs

- None

INVALID DOIs

- None

meg-simula commented 3 years ago

Hi @schruste,

Thanks again for the submission and for handling all reviewer feedback. I assume that the paper in the master branch is the relevant one.

Could you address the following editorial points:

[ ] Use Python, not python.
[ ] Spell out w.r.t. (with regard to)
[ ] Replace "facilities", perhaps by "functionality"?
[ ] Use .~ to avoid breaking white space (e.g.~e.g.)
[ ] Replace "advanced features beyond that:", by e.g. "advanced features beyond these, including:".

After addressing these points, could you:

[ ] Make a tagged release of your software, and list the version tag of the archived version here.
[ ] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
[ ] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.
[ ] Please list the DOI of the archived version here.

I can then move forward with accepting the submission.

meg-simula commented 3 years ago

/ooo July 1 until August 9

schruste commented 3 years ago

@meg-simula . Thank you for your reply!

Indeed the latest changes have been made to the master branch.

I applied your changes, but have problems with "Use .~ to avoid breaking white space (e.g.~e.g.)" as this doesn't behave in markdown the same way one would expect in latex, the tilde is rendered.

schruste 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:

schruste commented 3 years ago

Ok, I think we are done:

[x] we fixed the issue with the whitespaces.
[x] The release tag is joss
[x] This version is now on zenodo with DOI 10.5281/zenodo.5081124,

meg-simula commented 3 years ago

@whedon generate pdf

openjournals / joss-reviews