search

openjournals / joss-reviews

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

[REVIEW]: GEOS: A performance portable multi-physics 2 simulation framework for subsurface applications #6973

Open editorialbot opened 2 months ago

editorialbot commented 2 months ago

Submitting author: !--author-handle-->@rrsettgast<!--end-author-handle-- (Randolph Settgast) Repository: https://github.com/GEOS-DEV/GEOS Branch with paper.md (empty if default branch): docs/JOSS Version: v1.0.1 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @MakisH, @timokoch, @berenger-eu Archive: Pending

Status

status

Status badge code:

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

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

@MakisH & @timokoch & @berenger-eu, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @lucydot 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 ✨

Checklists

📝 Checklist for @MakisH

📝 Checklist for @timokoch

📝 Checklist for @berenger-eu

editorialbot commented 2 months ago

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

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 2 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1002/nag.2557 is OK
- 10.5281/zenodo.11396894 is OK
- 10.1109/P3HPC49587.2019.00012 is OK
- 10.1147/JRD.2019.2954403 is OK
- 10.1007/3-540-47789-6_66 is OK
- 10.1145/1089014.1089021 is OK
- 10.1137/19M1256117 is OK
- 10.1016/j.cma.2021.114111 is OK
- 10.1017/9781009157926 is OK

MISSING DOIs

- No DOI given, and none found for title: CHAI
- No DOI given, and none found for title: PETSc Web page
- No DOI given, and none found for title: GEOS Documentation

INVALID DOIs

- None
editorialbot commented 2 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=1.74 s (1421.6 files/s, 372497.9 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
C++                             485          24070          16278         115405
SVG                              14             33             70         105329
C/C++ Header                    678          28274          58573          99010
XML                             535           8653           4000          61400
CSV                              47              0              0          60400
reStructuredText                432           9918           7284          14614
Python                          114           3808           2529          13407
CMake                           106           1166            793           4579
XSD                               1              0           1830           3662
YAML                             18            185            271           1684
Bourne Shell                     10            121            112            549
Markdown                         10             80             15            404
Bourne Again Shell               13             99             63            383
Perl                              1             34             76            191
TeX                               2             16              0            120
JSON                              5              0              0            116
diff                              3              9             40             35
INI                               1              9              0             33
make                              1             15              3             21
Dockerfile                        1              1              4             12
CSS                               1              3              3             11
C Shell                           2              3             18              8
--------------------------------------------------------------------------------
SUM:                           2480          76497          91962         481373
--------------------------------------------------------------------------------

Commit count by author:

  1366  Randolph R. Settgast
   315  Christopher Sherman
   276  Benjamin Curtice Corbett
   214  Sergey Klevtsov
   197  Randolph Settgast
   185  Francois Hamon
   131  Nicola Castelletto
   129  Antoine Mazuyer
    99  Arturo Vargas
    89  Matteo Cusini
    77  Thomas Gazzola
    73  TotoGaz
    61  Matthias
    57  Pavel Tomin
    51  Brian Han
    47  Herve Gross
    46  Joshua White
    42  Jian Huang
    38  Sy-Tuan Nguyen
    36  Hui Wu
    33  Andrea Franceschini
    29  Ben Corbett
    27  Thomas GAZZOLA
    25  hannah_mairs
    24  Dickson Kachuma
    24  Xavier Lacoste
    20  tbeltzun
    19  MelReyCG
    19  William R Tobin
    19  Yue Hao
    19  acitrain
    18  Victor A. P. Magri
    17  Jixiang Huang
    17  Stefano Frambati
    16  Lionel Untereiner
    16  francoishamon
    11  Jacques Franc
    10  robinspb
    10  wrtobin
     8  Andrea Borio
     8  shabnamjs
     7  Madonna Yoder
     7  frankfeifan
     7  labesse40
     5  Peter B. Robinson
     5  Randy Settgast
     5  Uncrustify Robot
     5  mndiaye24
     4  Mark Khait
     4  Mohammad Karimi-Fard
     4  Tao Jin
     4  Terry Ligocki
     3  AntoineMazuyer
     3  Ben
     3  Brian Manh Hien Han
     3  Chris White
     3  Ryan Aronson
     3  WuHuiLLNL
     3  andrembcosta
     3  paloma-martinez
     3  sohailwaziri
     2  Arnaud DUDES
     2  Mamadou N'diaye
     2  Matteo
     2  Milad Bader
     2  Tom Byer
     2  Victor A. Paludetto Magri
     2  castelletto1
     2  cmcrook5
     2  jacques
     2  jiemeng-total
     2  mazuyer
     2  settgast1
     1  AlexandreLapene
     1  Andre Costa
     1  Benjamin Corbett
     1  Bertrand Thierry
     1  Castelletto
     1  Guotong
     1  Hewei Tang
     1  Isaac Ju
     1  Kevin J. Dugan
     1  Pengcheng Fu
     1  Quan Bui
     1  aure-lily
     1  dependabot[bot]
     1  geosadmn
     1  joshwhite
     1  kimtaeho07
     1  matteofrigo5
     1  mkhait
     1  rasimHZ
     1  tony
     1  Łukasz Łaniewski-Wołłk
editorialbot commented 2 months ago

Paper file info:

📄 Wordcount for paper.md is 1516

✅ The paper includes a Statement of need section

editorialbot commented 2 months ago

License info:

🟡 License found: Other (Check here for OSI approval)

editorialbot commented 2 months ago

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

lucydot commented 1 month ago

Availability through Docker images only would not necessarily be an acceptance-blocker but this would need to be made clear in the paper/docs.

@rrsettgast - this was recently discussed in an editorial meeting, where we decided that docker containers shouldn’t be the only installation pathway for a package for review. One reason for this is that researchers should be able to develop and contribute to open software without having to use Docker, which is more complicated for development.

@MakisH @timokoch @berenger-eu - please follow the installation instructions provided in the GEOS-2024 documentation. @rrsettgast - if you foresee problems with a particular installation route (in addition to the note on GPUs already given in the pre-review) please guide the reviewers appropriately. Two reviewers are away through part of July, so it may be that this review does not start in earnest for another two weeks.

lucydot commented 1 month ago

@rrsettgast @MakisH @timokoch @berenger-eu

A note here that from this Monday 29th I will be on leave (⛺) for just over two weeks, returning on the 14th of August when I'll check in on progress ASAP. If you have any queries, it may be best to ask before I go.

berenger-eu commented 1 month ago

I would like to use GEOS on a platform with MPI+CUDA, I opened an issue because I have difficulties to do so https://github.com/GEOS-DEV/GEOS/issues/3256

timokoch commented 1 month ago

Review checklist for @timokoch

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

timokoch commented 1 month ago

@rrsettgast The statement of need section is currently on the "shallow" side. The state of the field is currently not mentioned. CO2 injection scenarios (cited as the main goal but also "geothermal energy, hydrogen storage, and related subsurface applications") is of course a topic for many, with a large number of open source tools out there. You probably know of the 11th SPE CSP which has a large number of open source codes (and closed source codes) participating. The same goes for HPC multi-physics tools or coupling libraries. So I think it would be good to (a) mention other tools (at least the ones with a clear (partial) overlap of topics and capabilities, definitely tools like OPM, OpenGeoSys, just to name two FOSS with a very clear overlap) and (b) highlight the specific strengths of GEOS in comparison to those tools (I guess you have both great HPC capabilities and some complex THCM models, I also like the extensive benchmark/verification suite, ...).

MakisH commented 1 month ago

Review checklist for @MakisH

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

berenger-eu commented 3 weeks ago

Review checklist for @berenger-eu

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

berenger-eu commented 3 weeks ago

@rrsettgast as it has been stated by another reviewer, it would be great to add some references to competitors (i.e., other software doing related things). If this is updated, please could you regenerate the pdf? And I will check the last tick for my review. Thanks.

rrsettgast commented 3 weeks ago

@berenger-eu @timokoch I am working on a revision today. Thanks!

rrsettgast commented 3 weeks ago

@editorialbot generate pdf

editorialbot commented 3 weeks ago

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

berenger-eu commented 3 weeks ago

@rrsettgast thanks. Everything is fine for me now.

MakisH commented 3 weeks ago

@rrsettgast Some comments on the text (I still need to build and run the code):

Some typos and minor comments:

I have also added some notes on my checklist (cc @lucydot).

I have opened one PR to the documentation (https://github.com/GEOS-DEV/GEOS/pull/3289), I might open more at the end.

lucydot commented 3 weeks ago

Thanks for your thorough review so far @MakisH. I've noted your notes!

@rrsettgast, in case you missed it - there are three open issues to address listed under points in @MakisH's checklist.

rrsettgast commented 3 weeks ago

@MakisH Thank you for your thorough review and feedback. I will address the issues within the next couple of days.

MakisH commented 3 weeks ago

I was able to build and run the code on my Ubuntu 22.04 laptop (after upgrading CMake), with this host config:

# detect host and name the configuration file
site_name(HOST_NAME)
set(CONFIG_NAME "your-platform" CACHE PATH "")
message("CONFIG_NAME = ${CONFIG_NAME}")

# set paths to C, C++, and Fortran compilers. Note that while GEOS does not contain any Fortran code,
# some of the third-party libraries do contain Fortran code. Thus a Fortran compiler must be specified.
set(CMAKE_C_COMPILER "/usr/bin/gcc" CACHE PATH "")
set(CMAKE_CXX_COMPILER "/usr/bin/g++" CACHE PATH "")
set(CMAKE_Fortran_COMPILER "/usr/bin/gfortran" CACHE PATH "")
set(ENABLE_FORTRAN OFF CACHE BOOL "" FORCE)

# enable MPI and set paths to compilers and executable.
# Note that the MPI compilers are wrappers around standard serial compilers.
# Therefore, the MPI compilers must wrap the appropriate serial compilers specified
# in CMAKE_C_COMPILER, CMAKE_CXX_COMPILER, and CMAKE_Fortran_COMPILER.
set(ENABLE_MPI ON CACHE BOOL "")
set(MPI_C_COMPILER "/usr/bin/mpicc" CACHE PATH "")
set(MPI_CXX_COMPILER "/usr/bin/mpicxx" CACHE PATH "")
set(MPI_Fortran_COMPILER "/usr/bin/mpifort" CACHE PATH "")
set(MPIEXEC "/usr/bin/mpirun" CACHE PATH "")

# disable CUDA and OpenMP
set(ENABLE_CUDA OFF CACHE BOOL "" FORCE)
set(ENABLE_OPENMP OFF CACHE BOOL "" FORCE)

# enable PVTPackage
set(ENABLE_PVTPackage ON CACHE BOOL "" FORCE)

# enable tests
set(ENABLE_GTEST_DEATH_TESTS ON CACHE BOOL "" FORCE )

# define the path to your compiled installation directory
set(GEOS_TPL_DIR "/home/gc/Desktop/codes/thirdPartyLibs/install-laptop-release" CACHE PATH "")
# let GEOS define some third party libraries information for you
include(${CMAKE_CURRENT_LIST_DIR}/tpls.cmake)

All tests pass.

I could run the first tutorial and I am getting the same results:

Screenshot from 2024-08-20 22-03-27

I have not tried running anything else, but I am happy to try anything specific.

I opened an issue with some suggestions for improvements to the Quick Start Guide: https://github.com/GEOS-DEV/GEOS/issues/3291

lucydot commented 3 weeks ago

I am happy to try anything specific.

@MakisH thanks for your clear updates. I encourage you to confirm the main functional claims of the software. This might correspond to the four core tutorials, I'd probably play around with a few of the basic/advanced examples also depending on your capacity. There is so much on the website (in a good way!) that its certainly not possible to run through it all.

A quick note that I am on leave for the following week and will be back at desk on 2nd Sept.

rrsettgast commented 2 weeks ago

@editorialbot generate pdf

editorialbot commented 2 weeks ago

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

rrsettgast commented 2 weeks ago

@rrsettgast The statement of need section is currently on the "shallow" side. The state of the field is currently not mentioned. CO2 injection scenarios (cited as the main goal but also "geothermal energy, hydrogen storage, and related subsurface applications") is of course a topic for many, with a large number of open source tools out there. You probably know of the 11th SPE CSP which has a large number of open source codes (and closed source codes) participating. The same goes for HPC multi-physics tools or coupling libraries. So I think it would be good to (a) mention other tools (at least the ones with a clear (partial) overlap of topics and capabilities, definitely tools like OPM, OpenGeoSys, just to name two FOSS with a very clear overlap) and (b) highlight the specific strengths of GEOS in comparison to those tools (I guess you have both great HPC capabilities and some complex THCM models, I also like the extensive benchmark/verification suite, ...).

Hello @timokoch I think we have addressed this to some degree in the latest revisions. I have included references to OPM, OpenGeoSys, DuMux, and Darts. I kept the list somewhat short, but I think these are the open source codes that may be participating in SPE11.

There are some other unchecked items in your review. I can't seem to find specific comments from you on those topics. I will circle back with you once I address some of the specific comments made by @MakisH.

rrsettgast commented 2 weeks ago

@MakisH The Frontier results are indeed previously unpublished. We will be publishing results with more detail at a later date. However, that publication would focus more on the hypre solution strategy, and reference this paper ideally.

I have added more details about Frontier, and modified the discussion... but the runs themselves would be difficult to verify as Frontier is not accessible to the general public.

@lucydot Do you have guidance on whether some results can stay in this publication without an in-depth discussion? Perhaps a statement indicating that the results are presented simply to show execution on a large GPU system?

rrsettgast commented 2 weeks ago

@MakisH Regarding your comments about the lack of a C++ api. Does this mean that you don't consider the Doxygen documentation as a valid API documentation? Or was it just to hard to get to? For instance, the main data structure object Group is documented here: https://geosx-geosx.readthedocs-hosted.com/en/latest/doxygen_output/html/classgeos_1_1data_repository_1_1_group.html

Which I would think of as API documentation for that class. But there is no top level description with targeted entry points to the doxygen documentation in the develop guide. Perhaps This is what is missing for you?

lucydot commented 2 weeks ago

There are some other unchecked items in your review.

@rrsettgast - I think we can assume @timokoch is part way through the review, its quite usual for reviewers to do the review in parts rather than in a single-shot.

lucydot commented 2 weeks ago

@rrsettgast

Do you have guidance on whether some results can stay in this publication without an in-depth discussion? Perhaps a statement indicating that the results are presented simply to show execution on a large GPU system?

The important point here is Performance: If there are any performance claims of the software, have they been confirmed?. As you state, these cannot be confirmed as our reviewers do not have access to a suitable system. However it should be possible to include details on how to reproduce these results if someone did have access to the system. Ideally there would be a reference to the pre-print or similar which dives into the performance claims, but it sounds like that is not possible at this point. As you mention, we want to avoid in-depth discussion in the paper itself. Is it possible to include details within a benchmarking/performance section of the online documentation?

rrsettgast commented 2 weeks ago

@rrsettgast

Do you have guidance on whether some results can stay in this publication without an in-depth discussion? Perhaps a statement indicating that the results are presented simply to show execution on a large GPU system?

The important point here is Performance: If there are any performance claims of the software, have they been confirmed?. As you state, these cannot be confirmed as our reviewers do not have access to a suitable system. However it should be possible to include details on how to reproduce these results if someone did have access to the system. Ideally there would be a reference to the pre-print or similar which dives into the performance claims, but it sounds like that is not possible at this point. As you mention, we want to avoid in-depth discussion in the paper itself. Is it possible to include details within a benchmarking/performance section of the online documentation?

Ah. Yes. I certainly can include some guidance to reproduce in the documentation. Thanks!

MakisH commented 2 weeks ago

@MakisH Regarding your comments about the lack of a C++ api. Does this mean that you don't consider the Doxygen documentation as a valid API documentation? Or was it just to hard to get to? For instance, the main data structure object Group is documented here: https://geosx-geosx.readthedocs-hosted.com/en/latest/doxygen_output/html/classgeos_1_1data_repository_1_1_group.html

Which I would think of as API documentation for that class. But there is no top level description with targeted entry points to the doxygen documentation in the develop guide. Perhaps This is what is missing for you?

@rrsettgast the Doxygen pages are of course great as API documentation, but the webpage does not guide to the specific user-facing classes. As an external, first-time observer, I would need to guess that geos::dataRepository::Group is the main entry point. As a user, I primarily need that, and not any classes not exposed to the user.

Just adding such a summary on the webpage of where one should look at in the Doxygen pages would solve the issue.

P.S.: I will run a few more tutorials/examples asap.

MakisH commented 2 weeks ago

I ran the following cases:

With an external view (I am not an expert in this type of simulations), I can see that the software indeed simulates problems relevant to CO2 storage and generally injection of fluids into wells.

Tutorial 2 (with two meshes):

tutorial2hex

tutorial2tet

Tutorial 3:

tutorial3

CO2 injection:

co2-injection

Related PR: https://github.com/GEOS-DEV/GEOS/pull/3315

rrsettgast commented 1 week ago

@editorialbot generate pdf

editorialbot commented 1 week ago

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

rrsettgast commented 1 week ago

@MakisH @timokoch I think that we have addressed the points each of you have brought up. Please have a look and let us know if there is anything that we missed, or if there is anything else you notice that needs to be addressed.

@editorialbot generate pdf

rrsettgast commented 1 week ago

@editorialbot generate pdf

editorialbot commented 1 week ago

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

rrsettgast commented 1 week ago

@lucydot Let me know if I should do anything else...like provide a summary of the changes made to address each of the missing checkmarks.

MakisH commented 1 week ago

From my side, all points in the checklist are covered. Thank you for all the improvements!

I can confirm that at least the software seems to address the applications claimed (based on the documentation and examples, I am not working in this application field), which are indeed multi-physics applications. I can confirm that one can build and run the software with the provided documentation, and that there are sufficient pointers to getting support and to contributing.

The performance claims seem to be sufficiently documented: if someone had access to Frontier or a similar system, it is now clear which example to run and how. I did not run this example. However, I also don't think that JOSS is the right place to discuss such results, but including the results is appropriate, since the software emphasizes its suitability for HPC.

Looking at the paper again, I would like to draw the attention to the title:

GEOS-2024: A portable multi-physics simulation framework

I think that the "multi-physics simulation framework" is clear (it is a code framework in which one can set up multi-physics simulations). However, looking at lines 47-49:

However, GEOS stands out in two key areas: the explicit fault modeling coupled with flow and mechanical deformation, and the focus on performance portability on platforms ranging from workstations to exascale supercomputers.

rrsettgast commented 1 week ago

@MakisH Thank you again for spending your time on this. I understand your remaining points.

GEOS-2024: A performance portable multi-physics simulation framework for subsurface applications

@lucydot Are we allowed to modify the title?

timokoch commented 1 week ago

@rrsettgast Thanks for all the great updates. I fully support @MakisH comments/suggestions and I like your title suggestion since it more clearly states the current focus. I now completed the tutorials (it was fun, sorry that it took some time) and also successfully ran some examples. The tool is clearly useful, seems performant, is easy to configure, and covers a broad range of applications and models within the subsurface scope.

Some comments below

lucydot commented 1 week ago

@rrsettgast

Are we allowed to modify the title?

Yes, that should be no problem. Once you update the title on your paper let me know and I will update the issue title.

It seems you have a few very minor points to address above from @timokoch. I can see that all points in checkboxes are ✅ apart from one regarding references (@timokoch ?).

Once this is done, we can proceed to final stages of the review process.

I want to give a big thank you to @MakisH, @timokoch, @berenger-eu for your time and expertise. JOSS could not exist without your generosity!

rrsettgast commented 1 week ago

@timokoch I think I addressed your remaining points in https://github.com/GEOS-DEV/GEOS/pull/3332

rrsettgast commented 1 week ago

@editorialbot generate pdf

editorialbot commented 1 week ago

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

MakisH commented 6 days ago

@rrsettgast title looks great now, and not too long. Thank you!

@lucydot just let me know if you need anything else, happy to be doing this.

rrsettgast commented 6 days ago

@editorialbot generate pdf

editorialbot commented 6 days ago

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

lucydot commented 3 days ago

I can see some action in https://github.com/GEOS-DEV/GEOS/pull/3332 - can you confirm you are happy with changes made, and recommend acceptance @timokoch ?