search

openjournals / joss-reviews

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

[REVIEW]: SeismicMesh: Triangular meshing for seismology #2687

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @krober10nd (Keith Roberts) Repository: https://github.com/krober10nd/SeismicMesh Version: v3.3.1 Editor: @meg-simula Reviewers: @nschloe, @jorgensd Archive: 10.5281/zenodo.4447042

: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/ba94127ebbd0ca13c841f047fb5077bd"><img src="https://joss.theoj.org/papers/ba94127ebbd0ca13c841f047fb5077bd/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/ba94127ebbd0ca13c841f047fb5077bd/status.svg)](https://joss.theoj.org/papers/ba94127ebbd0ca13c841f047fb5077bd)

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

@nschloe, 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 @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 @nschloe

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @jorgensd

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @nschloe 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 4 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s00366-006-0014-1 is OK
- 10.1016/j.cageo.2007.06.014 is OK
- 10.1145/2998441 is OK
- 10.1007/s00158-018-1950-2 is OK
- 10.1007/s12665-015-4537-x is OK
- 10.1073/pnas.93.4.1591 is OK
- 10.5194/gmd-12-1847-2019 is OK
- 10.1016/j.jcp.2014.01.005 is OK
- 10.1109/SC.2014.86 is OK
- 10.1002/nme.2579 is OK
- 10.1145/2629697 is OK
- 10.1093/gji/ggv380 is OK
- 10.1190/1.3238367 is OK
- 10.1137/S0036144503429121 is OK
- 10.1190/1.1441754 is OK
- 10.1190/1.1437283 is OK

MISSING DOIs

- 10.1007/978-3-642-04319-2_10 may be a valid DOI for title: Perturbing slivers in 3D Delaunay meshes

INVALID DOIs

- None
whedon commented 4 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 4 years ago

Issue started by whedon before proper assignment of reviewers, closing this and trying again.

arfon commented 4 years ago

@whedon add @jorgensd as reviewer

whedon commented 4 years ago

OK, @jorgensd is now a reviewer

arfon commented 4 years ago

OK, you should be good to go here now @meg-simula!

meg-simula commented 4 years ago

Ok, thanks @arfon!

@nschloe @jorgensd @krober10nd Thanks for your patience - ready for the review now.

krober10nd commented 4 years ago

Thanks could you correct the title please? It says ERR ..

jorgensd commented 3 years ago

@arfon I am not able to tick the checklist boxes above, and when trying to accept the invitation I get that it is expired.

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

jorgensd commented 3 years ago

@whedon commands

whedon commented 3 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository
jorgensd commented 3 years ago

@arfon I am not able to tick the checklist boxes above, and when trying to accept the invitation I get that it is expired.

@arfon Im still not able to tick the boxes above (even now that Im assigned as reveiwer), is it possible to resend an invitiation to me?

arfon commented 3 years ago

@whedon re-invite @jorgensd as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@jorgensd please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

arfon commented 3 years ago

@jorgensd can you try accepting the invite again now? The invites should work for a week.

jorgensd commented 3 years ago

@whedon check repository

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.27 s (264.0 files/s, 33165.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          43            947            719           3504
C++                              6            264            153           1327
Markdown                         4            201              0            584
reStructuredText                 7            260            102            272
TeX                              1             22              0            230
YAML                             2              1              1             40
make                             2             14              8             37
DOS Batch                        1              8              1             26
CMake                            1             12              6             25
INI                              2              2              0             24
TOML                             1              0              0              3
-------------------------------------------------------------------------------
SUM:                            70           1731            990           6072
-------------------------------------------------------------------------------

Statistical information for the repository '449e4b4c398ffa4988367928' was
gathered on 2020/10/22.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Keith Roberts                  252         40560          33019           89.34
keith roberts                   43          4785           3723           10.33
krober10                         5           156            119            0.33

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
Keith Roberts              6847           16.9          2.6                8.69
krober10                     67           42.9          5.8                7.46
jorgensd commented 3 years ago

@krober10nd Could you elaborate on your co-authors contributions to this paper/software? By looking at the code contributions, it seems that all commits has been made by you.

krober10nd commented 3 years ago

@jorgensd Yes, you are correct. I made the technical contributions/implementations to this work and wrote the paper and I also maintain the software. Both Rafael and William offered their scientific/professional opinions and we had numerous intellectual discussions which led to new ideas and which I later pursued and implemented in the code stack. Both helped review and edit offline earlier versions of the program despite the fact they have not committed to the repository.

nschloe commented 3 years ago

Okay, I'm going over the text now. One thing I noticed is the following

krober10nd commented 3 years ago

Hey @nschloe

For the future, I think we should discuss this in the repository with an issue but I will answer this here briefly. We can start an issue and discuss in the repo.

"The usage of 11 cores reduces the generation time of this example from 20 minutes to approximately 2 wall-clock minutes". According to the graph, it's more "from 6 minutes". I would also like to see a dashed line in the graph for perfect speed up (or equal scaling in the axes.)

Sure. I can fix this.

Also a question: Why don't we have perfect speed-up?

There are two reasons: 1) the domain decomposition strategy that I use does not consider load balancing (it simply slices one axis and does not consider the spatial distribution of points) 2) the communication cost becomes competitive to the parallelization speed-up of the Delaunay triangulation.

krober10nd commented 3 years ago

Just an update: I remade the parallel figure, added it to the paper, and updated the text.

Python axpy in parallel indeed gets in the way. Something always to be improved.

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

jorgensd 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.1007/s00366-006-0014-1 is OK
- 10.1016/j.cageo.2007.06.014 is OK
- 10.1145/2998441 is OK
- 10.1007/s00158-018-1950-2 is OK
- 10.1007/s12665-015-4537-x is OK
- 10.1073/pnas.93.4.1591 is OK
- 10.5194/gmd-12-1847-2019 is OK
- 10.1016/j.jcp.2014.01.005 is OK
- 10.1109/SC.2014.86 is OK
- 10.1002/nme.2579 is OK
- 10.1145/2629697 is OK
- 10.1093/gji/ggv380 is OK
- 10.1190/1.3238367 is OK
- 10.1137/S0036144503429121 is OK
- 10.1190/1.1441754 is OK
- 10.1190/1.1437283 is OK

MISSING DOIs

- 10.1007/978-3-642-04319-2_10 may be a valid DOI for title: Perturbing slivers in 3D Delaunay meshes

INVALID DOIs

- None
jorgensd commented 3 years ago

@krober10nd It seems to me that the cgal reference in the Performance Comparison chapter is broken. In the article proof above it shows up as: The 2D/3D serial performance against gmsh and cgal (Alliez et al., 2020, pp. @cgal:r–ctm2– 20b)

krober10nd commented 3 years ago

Okay I put in a fix for this citation and removed the RTM acronym @jorgensd . I'll update the benchmark figure with hopefully bigger labels and the clarification about the 2D performance of SM.

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

jorgensd commented 3 years ago

@krober10nd As whedon is suggesting, you are missing the DOI of the Tournois paper in the references. The DOI is: 10.1007/978-3-642-04319-2_10

krober10nd commented 3 years ago

@whedon check references

whedon commented 3 years ago

Checking the BibTeX entries failed with the following error:

Failed to parse BibTeX on value "doi" (NAME) [#, "@", #, {:title=>["Perturbing slivers in 3D Delaunay meshes"], :author=>["Tournois, Jane and Srinivasan, Rahul and Alliez, Pierre"], :booktitle=>["Proceedings of the 18th international meshing roundtable"], :pages=>["157--173"], :year=>["2009"], :publisher=>["Springer"]}]

krober10nd 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.1007/s00366-006-0014-1 is OK
- 10.1016/j.cageo.2007.06.014 is OK
- 10.1145/2998441 is OK
- 10.1007/s00158-018-1950-2 is OK
- 10.1007/s12665-015-4537-x is OK
- 10.1073/pnas.93.4.1591 is OK
- 10.5194/gmd-12-1847-2019 is OK
- 10.1016/j.jcp.2014.01.005 is OK
- 10.1109/SC.2014.86 is OK
- 10.1002/nme.2579 is OK
- 10.1145/2629697 is OK
- 10.1093/gji/ggv380 is OK
- 10.1190/1.3238367 is OK
- 10.1137/S0036144503429121 is OK
- 10.1190/1.1441754 is OK
- 10.1007/978-3-642-04319-2_10 is OK
- 10.1190/1.1437283 is OK

MISSING DOIs

- None

INVALID DOIs

- None
meg-simula commented 3 years ago

Hi @krober10nd @nschloe @jorgensd Just checking in to see how you are doing - let me know if there is anything I can contribute with in this on-going review. Thanks again for your efforts.

krober10nd commented 3 years ago

Thanks @meg-simula I've been busy improving the paper incorporating the reviewers suggestions. The reviews have been very productive in my opinion.

nschloe commented 3 years ago

I agree. This is still an ongoing effort.

krober10nd commented 3 years ago

@nschloe @jorgensd Can you please elaborate more on what ongoing work needs to be done for to finish this review?

Just to state some recent activity: I've fixed a broken reference in the paper and edited a statement regarding mesh quality on KSP performance. I've improved both the serial and parallel performance comparisons and at least the serial performance comparisons have been corroborated via Nico's meshgen comparison repository.

Other than that, I've also improved the README to follow the guidelines of the JOSS review by adding a contributing section and clarified the statement of need.

Thank you

jorgensd commented 3 years ago

@nschloe @jorgensd Can you please elaborate more on what ongoing work needs to be done for to finish this review?

Just to state some recent activity: I've fixed a broken reference in the paper and edited a statement regarding mesh quality on KSP performance. I've improved both the serial and parallel performance comparisons and at least the serial performance comparisons have been corroborated via Nico's meshgen comparison repository.

Other than that, I've also improved the README to follow the guidelines of the JOSS review by adding a contributing section and clarified the statement of need.

Thank you

I think we are getting quite close to a finalized version. I've added some more notes to the various issues in SeismicMesh (and some of them you have already addressed). When we iron out the last few things, I am happy with the state of the software. The final thing would be to have a last read-through of the paper. Nico has given you some good input, and I believe that I will have very few corrections after the thread https://github.com/krober10nd/SeismicMesh/issues/139 is finalized.

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

krober10nd commented 3 years ago

I fixed the CGAL reference above. Now there's a typsetting issue remaining.

I made the benchmark figure quite long to deal with the fact that the JOSS article format has large margins but now the caption of that figure is all jumbled up.

I guess the solution is to make the figure wider now.

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

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