search

openjournals / joss-reviews

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

[PRE REVIEW]: Mandyoc: A finite element code to simulate thermochemical convection in parallel #3551

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: @victorsacek (Victor Sacek) Repository: https://github.com/ggciag/mandyoc Version: v0.1.1 Editor: @jedbrown Reviewers: @rbeucher Managing EiC: Kyle Niemeyer

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

Author instructions

Thanks for submitting your paper to JOSS @victorsacek. Currently, there isn't an JOSS editor assigned to your paper.

@victorsacek if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

Editor instructions

The JOSS submission bot @whedon is here to help you find and assign reviewers and start the main review. To find out what @whedon can do for you type:

@whedon commands
whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks.

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

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

Wordcount for paper.md is 817

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.12 s (490.0 files/s, 96813.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             14           2303            350           5185
Python                          13            371            316            718
TeX                              2             34              0            342
Markdown                         6             82              0            248
reStructuredText                 9            343            397            247
C/C++ Header                     1             91             10            244
make                             2             16              4             64
YAML                             1              9             21             35
Bourne Shell                     9              1              2             16
CSS                              1              1              0              9
-------------------------------------------------------------------------------
SUM:                            58           3251           1100           7108
-------------------------------------------------------------------------------

Statistical information for the repository '76ee3286be0dee889ffe98a5' was
gathered on 2021/07/29.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Agustina                         1             1              1            0.01
Agustina Pesce                   1            17             10            0.20
Dave May                         1             2              0            0.01
Jamison Assunção                 3           716            700           10.40
Rafael Monteiro da S             3           391              4            2.90
Rafael Silva                     4           907             11            6.75
Victor Sacek                    69          9325           1524           79.72

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
Agustina                     18         1800.0          1.4                0.00
Dave May                      2          100.0         16.0                0.00
Jamison Assunção            701           97.9          0.9               22.11
Rafael Monteiro da S       1175          300.5          6.3                4.60
Victor Sacek               7692           82.5          3.4                3.95
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1051/0004-6361/201629272 is OK
- 10.1051/0004-6361/201322068 is OK

MISSING DOIs

- 10.1111/j.1365-246x.2012.05388.x may be a valid DOI for title: A comparison of numerical surface topography calculations in geodynamic modelling: an evaluation of the ‘sticky air’method
- 10.1016/j.pepi.2010.04.007 may be a valid DOI for title: A stabilization algorithm for geodynamic numerical simulations with a free surface
- 10.1007/978-1-4612-1986-6_8 may be a valid DOI for title: Efficient Management of Parallelism in Object Oriented Numerical Software Libraries
- 10.1016/j.epsl.2016.11.026 may be a valid DOI for title: Post-rift influence of small-scale convection on the landscape evolution at divergent continental margins
- 10.1016/j.jog.2021.101830 may be a valid DOI for title: Lateral flow of thick continental lithospheric mantle during tectonic quiescence

INVALID DOIs

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

kyleniemeyer commented 3 years ago

@victorsacek thanks for your interest in JOSS! Our editors in this area are at capacity right now, so I'm going to place this on our waitlist until someone becomes available.

kyleniemeyer commented 3 years ago

@jedbrown I think you are at editing capacity right now, but wanted to give you a heads-up: this looks to be right in your wheelhouse.

jedbrown commented 3 years ago

@whedon assign @jedbrown as editor

whedon commented 3 years ago

OK, the editor is @jedbrown

jedbrown commented 3 years ago

Thanks for this submission. While I locate reviewers, could I ask that you update the project documentation to avoid the many places where developer paths are hard-coded (and thus won't work for users). I think most of these can be removed rather than "fixed", and the documentation can state how to run the code without these scripts. (You could audit these steps in a container or a third-party computer to ensure the instructions make sense and actually work.)

$ git grep /Users
examples/continental_rift/run.sh:/Users/victorsacek/Documents/petsc/arch-label-debug/bin/mpirun -n 8 \
examples/continental_rift/run.sh:/Users/victorsacek/Documents/gits/md2d/md2d_aux/mandyoc \
examples/old_version/Crameri2012_Case2/run.sh:nohup /Users/victorsacek/Documents/petsc/arch-label-debug/bin/mpirun -n 4 ./mandyoc -denok 1.0e-15 -rtol 1.0e-7 -particles_per_ele 1000 -theta_FSSA 0.5 -sub_division_time_step 1.0 -visc_harmonic_mean 0 -particles_perturb_factor 0 -visc_const_per_element 1 -pressure_in_rheol 1 <FD.in >FD.out &
examples/old_version/vanKeken1997/run.sh:nohup /Users/victorsacek/Documents/petsc/arch-label-debug/bin/mpirun -n 4 ./mandyoc -denok 1.0e-15 -particles_per_ele 1000 -theta_FSSA 0.5 -sub_division_time_step 1.0 -visc_harmonic_mean 0 -particles_perturb_factor 0 -visc_const_per_element 1 -pressure_in_rheol 1 <FD.in >FD.out &
examples/old_version/vanKeken1997_nondim/run-mg.sh:nohup /Users/victorsacek/Documents/petsc/arch-label-debug/bin/mpirun -n 4 ./mandyoc -denok 1.0e-15 -particles_per_ele 1000 -theta_FSSA 0.5 -sub_division_time_step 1.0 -visc_harmonic_mean 0 -particles_perturb_factor 0 -visc_const_per_element 1 -direct_solver 0 -pressure_in_rheol 1 -veloc_ksp_monitor_true_residual -veloc_ksp_type fgmres -veloc_pc_type mg -veloc_pc_mg_galerkin -veloc_pc_mg_levels 3 -veloc_mg_levels_ksp_max_it 8 <FD.in >FD.out &
examples/old_version/vanKeken1997_nondim/run.sh:nohup /Users/victorsacek/Documents/petsc/arch-label-debug/bin/mpirun -n 4 ./mandyoc -denok 1.0e-15 -particles_per_ele 1000 -theta_FSSA 0.5 -sub_division_time_step 1.0 -visc_harmonic_mean 0 -particles_perturb_factor 0 -visc_const_per_element 1 -direct_solver 0 -pressure_in_rheol 1 <FD.in >FD.out &
examples/vanKeken1997_case1a/run.sh:/Users/kugelblitz/opt/petsc/arch-0-fast/bin/mpirun -n 2 /Users/kugelblitz/Desktop/mandyoc-misc/mandyoc/mandyoc
src/compile.sh:make all PETSC_DIR=/Users/victorsacek/Documents/petsc PETSC_ARCH=arch-label-debug
src/compile_opt.sh:make all PETSC_DIR=/Users/victorsacek/Documents/petsc PETSC_ARCH=arch-label-optimized

In the paper, please also explain its positioning relative to other software in this space. You don't have to be comprehensive, but please communicate enough about why a researcher would choose this over a representative set of well-known packages.

The equations can be formatted with \begin{align} or \begin{gather} rather than a sequence of display maths.

I see the acknowledgement mentions multigrid, but the van Keken benchmark is configured to use MUMPS. A note on solvers would be useful (even if the intent is that expert users tune using PETSc options). The paper also doesn't mention whether it supports 3D problems and at what scale it has been used. The code isn't entirely helpful in that "future 3d version" appears in comments, while some functions say they are 3d.

Do you have automated tests? I see you have a GitHub action to build the docs, but not to test correctness. Some automated tests should be provided.

Have you used a code coverage tool to see if there is significant dead code?

Thank you for the pleasant build experience. There were a few -Wunused-but-set-variable warnings that may as well be fixed.

aguspesce commented 3 years ago

Hello @jedbrown, Thank you for taking your time to make the review! We are working to fix and improve the observation that you mentioned.

jedbrown commented 3 years ago

@aguspesce Great! Just let reply here after making updates. You can comment @whedon generate pdf to spin a new PDF at that time.

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

victorsacek commented 3 years ago

Hello @jedbrown,

Thank you very much for the review of our code.

We've made almost all the changes that you suggested:

We have to clarify that we decided not to create an automated test using GitHub actions because installing all dependencies to run Mandyoc and test it would take too much time for the CI. But we created a test based on regression testing. This test can be executed once the program is installed. We intend to improve the code to use Docker in the future.

All the changes are in the release v0.1.2.

Please inform us if the present version of the code is ok for review in JOSS.

Thank you for your attention.

@aguspesce @jamisonassuncao @rafaelmds

aguspesce commented 2 years ago

Hello @jedbrown, is there any update on the pre review process of this submission?

cc @danielskatz

aguspesce commented 2 years ago

Hello @jedbrown, is there any update on the pre review process of this submission?

cc @danielskatz

jedbrown commented 2 years ago

@aguspesce Yikes, I'm terribly sorry about my silence. Thanks for your effort here; it looks good. I see CONTRIBUTING.md says to run make test, but it actually needs make test_mandyoc (or change the Makefile target). The scripts also look inconsistent in where they get mpirun (PETSc directory or $MPI_PATH). It's probably best to make this a run-time option, but you have some choices

Here's a patch to make the test run out of the box, but the scripts still use the fragile style.

diff --git i/Makefile w/Makefile
index e0e91de..3d2f3e6 100644
--- i/Makefile
+++ w/Makefile
@@ -26,8 +26,6 @@ SOURCEC = $(SRC)/main.cpp \
    $(SRC)/sp.cpp
 OBJECTS = $(SOURCEC:%.cpp=%.o)

-MPI_PATH = ${PETSC_DIR}/${PETSC_ARCH}/bin
-
 help:
    @echo ""
    @echo "Commands:"
@@ -40,7 +38,7 @@ help:
 test_mandyoc:

    @echo "Run MANDYOC test may take several minutes.."
-   cd test/testing_data/ ; ${MPI_PATH}/mpirun -n 2 ../../mandyoc
+   cd test/testing_data/ ; ${MPIEXEC} -n 2 ../../mandyoc
    pytest -v test/testing_result.py 

 # Build Mandyoc

I ran this and get one test failure (0-velocity):


field = 'velocity', step = 0

    @pytest.mark.parametrize("field", fields)
    @pytest.mark.parametrize("step", steps)
    def test_result(field, step):
        """ """
        filename = f"{field}_{step}" + ".txt"
        output = read(test_path / filename)
        expected = read(expected_path / filename)

>       npt.assert_allclose(output, expected, rtol=1e-5)
E       AssertionError:
E       Not equal to tolerance rtol=1e-05, atol=0
E
E       Mismatched elements: 1 / 94978 (0.00105%)
E       Max absolute difference: 1.e-16
E       Max relative difference: 1.10695082e-05
E        x: array([0., 0., 0., ..., 0., 0., 0.])
E        y: array([0., 0., 0., ..., 0., 0., 0.])

test/testing_result.py:57: AssertionError

I'll find reviewers.

jedbrown commented 2 years ago

@victorsacek @aguspesce Could you please include in your statement of need some related packages to help a prospective user understand the relative merits of mandyoc versus other software packages in this domain?

jedbrown commented 2 years ago

@gassmoeller @rbeucher :wave: Would you be available to review this submission to JOSS?

victorsacek commented 2 years ago

Hello @jedbrown

Thank you very much for your attention.

We modified the Makefile, changing the MPI variables for the test, as you suggested.

Additionally, in the Statement of need, we added one sentence explaining that one advantage of Mandyoc is the possibility to incorporate variable boundary conditions in space and time, appropriate to simulate different pulses of tectonism in one model run.

About the failure test, we tried different machines but we couldn't reproduce this failure. Please, if possible, could you send us more details about the problem?

Best regards

rbeucher commented 2 years ago

Hi @jedbrown,

Yes I can review this submission. Looks interesting.

R

gassmoeller commented 2 years ago

Hi @jedbrown thanks for asking. This would normally be right in my area of interest, but I am pretty tied up at the moment and would not get to the review before mid March. Maybe @bangerth or @tjhei are available?

jedbrown commented 2 years ago

@victorsacek Thanks for your updates. I think you'll find that other software supports BCs that depend on space and time. I think this section should be specific in placing present work in the context of related work. In this case, I think it involves citing some alternatives and explaining the relative merits.

Also, in the Makefile, it's better not to change MPIEXEC, which has already been defined in the makefile you included. Your redefinition prevents use of system MPI (i.e., when not using --download-mpich or --download-openmpi).

Thanks for being willing to review, @rbeucher! I'll add you and start the review. I have some emails out and will find a second reviewer as well. Of course I'd be thrilled if @bangerth or @tjhei are available. And I totally understand this present state of being burried @gassmoeller; thanks for your reply.

jedbrown commented 2 years ago

@whedon add @rbeucher as reviewer

whedon commented 2 years ago

OK, @rbeucher is now a reviewer

jedbrown commented 2 years ago

@whedon start review

whedon commented 2 years ago

OK, I've started the review over in https://github.com/openjournals/joss-reviews/issues/4070.

rafaelmds commented 2 years ago

@whedon check references

whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1111/j.1365-246X.2012.05388.x is OK
- 10.1016/j.pepi.2010.04.007 is OK
- 10.1007/978-1-4612-1986-6_8 is OK
- 10.1016/j.epsl.2016.11.026 is OK
- 10.1016/j.jog.2021.101830 is OK
- 10.1029/97JB01353 is OK

MISSING DOIs

- None

INVALID DOIs

- None