search

openjournals / joss-reviews

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

[REVIEW]: Motorcycle: A spectral boundary-integral method for seismic cycles on multiple faults #5097

Closed editorialbot closed 9 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@sbarbot<!--end-author-handle-- (Sylvain Barbot) Repository: https://bitbucket.org/sbarbot/motorcycle/src/master/ Branch with paper.md (empty if default branch): Version: v1.0 Editor: !--editor-->@jedbrown<!--end-editor-- Reviewers: @mherman09, @willic3, @thehalfspace Archive: 10.5281/zenodo.10129331

Status

status

Status badge code:

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

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

@mherman09 & @willic3, 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 @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 ✨

Checklists

📝 Checklist for @mherman09

📝 Checklist for @willic3

📝 Checklist for @thehalfspace

editorialbot commented 1 year 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 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1785/0120210004 is OK
- 10.1016/j.tecto.2019.05.004 is OK
- 10.1029/2021JB023519 is OK
- 10.1016/j.epsl.2021.117037 is OK
- 10.1016/j.epsl.2022.117593 is OK
- 10.1029/2008JB005934 is OK
- 10.1785/0120210178 is OK
- 10.1002/2016JB012857 is OK
- 10.1002/2016JB013778 is OK
- 10.1016/j.tecto.2019.228171 is OK
- 10.1029/2007JB004930 is OK
- 10.1115/1.4005896 is OK
- 10.1029/2020JB020865 is OK
- 10.1029/2021JB023726 is OK
- 10.1186/s40623-021-01465-6 is OK
- 10.1186/s40623-022-01649-8 is OK
- 10.1186/s40623-019-1113-8 is OK
- 10.1126/science.1218796 is OK
- 10.1029/2019JB018557 is OK
- 10.1109/JPROC.2004.840301 is OK
- 10.1029/2019GC008515 is OK
- 10.1109/38.56302 is OK
- 10.1063/1.4823180 is OK
- 10.1007/978-3-540-30218-6_19 is OK
- 10.1029/2022JB025106 is OK

MISSING DOIs

- 10.1109/mcc.1998.736436 may be a valid DOI for title: Numerical recipes in Fortran 90 the art of parallel scientific computing

INVALID DOIs

- None
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.22 s (301.9 files/s, 101588.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Fortran 90                      29           2956           3577           9983
Bourne Shell                    24            315            484           3195
Perl                             1            108             44            499
Markdown                         5            217              0            294
TeX                              1             27              0            265
make                             3             35              0             70
YAML                             1              2             13             54
MATLAB                           1              8              2             33
Fortran 77                       1              0              9             21
-------------------------------------------------------------------------------
SUM:                            66           3668           4129          14414
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 740

editorialbot commented 1 year ago

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

jedbrown commented 1 year ago

@mherman09 @willic3 :wave: Welcome to JOSS and thanks for agreeing to review! The comments from @editorialbot above outline the review process, which takes place in this thread (possibly with issues filed in the Motorcycle 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, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention this issue 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 aim for reviews to be completed within a month or two. Please let me know if you require some more time. We can also use editorialbot 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.

willic3 commented 1 year ago

Review checklist for @willic3

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

mherman09 commented 1 year ago

Review checklist for @mherman09

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

kthyng commented 1 year ago

Hi @mherman09, @willic3! What is the state of your reviews? Are you waiting on something from @sbarbot?

willic3 commented 1 year ago

Dear Kristen,

I sincerely apologise, but this had completely fallen through the cracks for me. I will try to get this done within the next couple of weeks.

Thanks, Charles

Charles Williams I Geodynamic Modeler GNS Science I Te Pῡ Ao 1 Fairway Drive, Avalon 5010, PO Box 30368, Lower Hutt 5040, New Zealand Ph 0064-4-570-4566 I Mob 0064-22-350-7326 I Fax 0064-4-570-4600 http://www.gns.cri.nz/ I Email: @.**@.>

On 19/04/2023, at 1:40 AM, Kristen Thyng @.***> wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe:

Hi @mherman09https://github.com/mherman09, @willic3https://github.com/willic3! What is the state of your reviews? Are you waiting on something from @sbarbothttps://github.com/sbarbot?

— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5097#issuecomment-1513180382, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABJHOVJJYXKR3LNDKVW6F5DXB2KWJANCNFSM6AAAAAAUEMZM7E. You are receiving this because you were mentioned.

Notice: This email and any attachments are confidential and may not be used, published or redistributed without the prior written consent of the Institute of Geological and Nuclear Sciences Limited (GNS Science). If received in error please destroy and immediately notify GNS Science. Do not copy or disclose the contents.

mherman09 commented 1 year ago

Hi Kristen,

Thank you for the reminder. I have been buried by teaching this semester. I have started on the technical review, but will need some additional time to complete it. Hopefully, like Charles, I will be able to do the review in the next few weeks. I will put additional reminders on my calendar!

matt

On Tue, Apr 18, 2023 at 6:40 AM Kristen Thyng @.***> wrote:

Hi @mherman09 https://github.com/mherman09, @willic3 https://github.com/willic3! What is the state of your reviews? Are you waiting on something from @sbarbot https://github.com/sbarbot?

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/5097#issuecomment-1513180382, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACFJARHMN574IXQ32JDLSGTXB2KWLANCNFSM6AAAAAAUEMZM7E . You are receiving this because you were mentioned.Message ID: @.***>

kthyng commented 1 year ago

Ok thank you @willic3 and @mherman09! @jedbrown or I will try to pop back in with some friendly reminders too.

willic3 commented 1 year ago

Dear Kristen,

I was hoping to be able to work on this, but with my upcoming schedule I don’t see any way I can fit it in. I am leaving for the USA on 26 May and I have a huge list of things to finish before I leave. I will not be back at work until 11 July. I doubt that the review could wait that long, but I would be willing to work on it when I return in July.

I am very sorry for this, but things have kept piling up.

Cheers, Charles

Charles Williams I Geodynamic Modeler GNS Science I Te Pῡ Ao 1 Fairway Drive, Avalon 5010, PO Box 30368, Lower Hutt 5040, New Zealand Ph 0064-4-570-4566 I Mob 0064-22-350-7326 I Fax 0064-4-570-4600 http://www.gns.cri.nz/ I Email: @.**@.>

On 19/04/2023, at 1:40 AM, Kristen Thyng @.***> wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe:

Hi @mherman09https://github.com/mherman09, @willic3https://github.com/willic3! What is the state of your reviews? Are you waiting on something from @sbarbothttps://github.com/sbarbot?

— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5097#issuecomment-1513180382, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABJHOVJJYXKR3LNDKVW6F5DXB2KWJANCNFSM6AAAAAAUEMZM7E. You are receiving this because you were mentioned.

Notice: This email and any attachments are confidential and may not be used, published or redistributed without the prior written consent of the Institute of Geological and Nuclear Sciences Limited (GNS Science). If received in error please destroy and immediately notify GNS Science. Do not copy or disclose the contents.

kthyng commented 1 year ago

@willic3 Thanks for letting us know. @jedbrown could you work on finding a replacement reviewer?

sbarbot commented 1 year ago

Hi All,

May I suggest Olaf Zielke. He and colleague Martin Mai at KAUST just released a similar code called MCQsim. See https://pubs.geoscienceworld.org/ssa/bssa/article-abstract/doi/10.1785/0120220248/622487/MCQsim-A-Multicycle-Earthquake-Simulator

Olaf's email is @.**@.>

I don't have any previous or ongoing collaboration with him.

Best wishes, Sylvain

From: Kristen Thyng @.> Sent: Wednesday, May 3, 2023 7:46:04 AM To: openjournals/joss-reviews @.> Cc: Sylvain Barbot @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: Motorcycle: A spectral boundary-integral method for seismic cycles on multiple faults (Issue #5097)

@willic3https://urldefense.com/v3/__https://github.com/willic3__;!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHAXyKN1yI$ Thanks for letting us know. @jedbrownhttps://urldefense.com/v3/__https://github.com/jedbrown__;!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHA8mjCnGo$ could you work on finding a replacement reviewer?

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/openjournals/joss-reviews/issues/5097*issuecomment-1533159471__;Iw!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHAATbOHII$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABJHTWDZRKSWOBZKH7KB3JLXEJVSZANCNFSM6AAAAAAUEMZM7E__;!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHA8rDdqlY$. You are receiving this because you were mentioned.Message ID: @.***>

sbarbot commented 1 year ago

Hi All,

I would also recommend the following computationally inclined young people with experience in numerical modeling of seismic cycles:

Jiayi Ye, Swiss Seismological Service (SED), ETH Zürich, @. Betti Hegyi, Structural Geology & Tectonics ETH Zürich, @. You Wu, Computational Geomechanics ETH Zürich, @.***

Other more senior people with relevant experience include:

Prithvi Thakur, University of Michigan, @. Mohamed Abdelmeguid, California Institute of Technology, @.

Best wishes, Sylvain

From: Sylvain Barbot @.***> Sent: Wednesday, May 3, 2023 8:42 AM To: openjournals/joss-reviews; openjournals/joss-reviews Cc: Mention Subject: Re: [openjournals/joss-reviews] [REVIEW]: Motorcycle: A spectral boundary-integral method for seismic cycles on multiple faults (Issue #5097)

Hi All,

May I suggest Olaf Zielke. He and colleague Martin Mai at KAUST just released a similar code called MCQsim. See https://pubs.geoscienceworld.org/ssa/bssa/article-abstract/doi/10.1785/0120220248/622487/MCQsim-A-Multicycle-Earthquake-Simulator

Olaf's email is @.**@.>

I don't have any previous or ongoing collaboration with him.

Best wishes, Sylvain

From: Kristen Thyng @.> Sent: Wednesday, May 3, 2023 7:46:04 AM To: openjournals/joss-reviews @.> Cc: Sylvain Barbot @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: Motorcycle: A spectral boundary-integral method for seismic cycles on multiple faults (Issue #5097)

@willic3https://urldefense.com/v3/__https://github.com/willic3__;!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHAXyKN1yI$ Thanks for letting us know. @jedbrownhttps://urldefense.com/v3/__https://github.com/jedbrown__;!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHA8mjCnGo$ could you work on finding a replacement reviewer?

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/openjournals/joss-reviews/issues/5097*issuecomment-1533159471__;Iw!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHAATbOHII$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABJHTWDZRKSWOBZKH7KB3JLXEJVSZANCNFSM6AAAAAAUEMZM7E__;!!LIr3w8kk_Xxm!s90oknhoy1iub9DMKk6e7R-yLgEAIbvto5erVnH9V4vA97WausLGrsfWMg5vieY1k2xHtPOaifDeAXHA8rDdqlY$. You are receiving this because you were mentioned.Message ID: @.***>

kthyng commented 1 year ago

@sbarbot Thank you for these suggestions! As long as they are not conflicted with you — can you provide their github usernames (without "@")? Their email addresses have been filtered out and github will be better anyway.

jedbrown commented 1 year ago

(I had started sending emails and will continue based on responses/lack of responses.)

kthyng commented 1 year ago

Ah sorry to step on toes. I'll step out of this now but just @ me if needed.

mherman09 commented 1 year ago

@sbarbot I was finally able to install and start running Motorcycle. My semester is over so I hope to have this review to you in the next week. I am really sorry for the delay. One quick question: I am not familiar with using Bitbucket - what is the best way to open issues there?

sbarbot commented 1 year ago

Thank you for your time and efforts. I truly appreciate it. I have activated the issue tracker on Bitbucket. You can now simply click on "Issues" on the left panel and then "Create issue". I look forward to your comments.

mherman09 commented 1 year ago

The motorcycle package is excellent so far - installation is generally smooth, the examples work as advertised, and I was able to set up and run my own models building off the examples. I brought up four minor issues as I was working through the checklist up to this point:

  1. The instructions for compiling the software needed some cleaning up, especially related to parallel versus serial versions of the executables (Issue #1)
  2. There are example scripts for running models with temperature-dependent rheologies (e.g., using the executable motorcycle-ap-ratestate-bath-serial), but I could not find any way to compile the executables (Issue #2)
  3. Although I love the programmatic way to define fault patch properties in the example scripts, I think it would be helpful for users to provide some additional in-line documentation within the awk command (Issue #3)
  4. Per the Review Checklist: “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.” I could not find these guidelines anywhere in the motorcycle repository. (Issue #4)
jedbrown commented 1 year ago

@editorialbot add @thehalfspace as reviewer

editorialbot commented 1 year ago

@thehalfspace added to the reviewers list!

jedbrown commented 1 year ago

:wave: Welcome @thehalfspace! Please see the top of the issue and this comment for instructions.

You can make a comment with the following line to get started:

@editorialbot generate my checklist
sbarbot commented 1 year ago

@mherman09 I have addressed issues #1, #3, and #4, but I cannot reproduce issue #2. I can compile and run motorcycle-ap-ratestate-bath-serial. Could you please double-check?

mherman09 commented 1 year ago

I double-checked, and I am not able to compile motorcycle-ap-ratestate-bath-serial. I am in the directory motorcycle/2d/antiplane, and edited my makefile for my brew installation to look like this:

# make file for compilation with brew in serial

INCPATH=-I/usr/local/include
LIBS=-L/usr/local/lib -lnetcdff -lfftw3_omp -lfftw3 -lm

FC=gfortran-13
CC=gcc-13

motorcycle-ap-ratestate-bath-serial: EXTRA=-DBATH

FFLAGS=$(INCPATH) -cpp -Wall -DNETCDF -DODE45 -fopenmp -finit-local-zero -ffree-line-length-none \
       -O3 -ffast-math -w -fallow-argument-mismatch $(EXTRA)
CFLAGS=$(INCPATH)

SRC=src-serial
DST=build

include common.mk

Typing make motorcycle-ap-ratestate-serial works fine, and the executable generates the expected output in the test scripts. Typing make motorcycle-ap-ratestate-bath-serial leads to many compiler errors related to variables not being present in the Fortran structures simulation_struct and patch_element_struct:

10:16:25 antiplane $ make motorcycle-ap-ratestate-bath-serial
gfortran-13 -I/usr/local/include -cpp -Wall -DNETCDF -DODE45 -fopenmp -finit-local-zero -ffree-line-length-none -O3 -ffast-math -w -fallow-argument-mismatch -DBATH  -c src-serial/ratestate_bath.f90 -o build/ratestate_bath.o -J build
src-serial/ratestate_bath.f90:243:33:

  243 |   ALLOCATE(in%profileTemperature(in%nFault))
      |                                 1
Error: 'profiletemperature' at (1) is not a member of the 'simulation_struct' structure
src-serial/ratestate_bath.f90:245:27:

  245 |      in%profileTemperature(i)%rate=exportNetcdfRate
      |                           1
Error: 'profiletemperature' at (1) is not a member of the 'simulation_struct' structure

(I did not copy all the errors here for brevity, but I can provide them as needed.)

sbarbot commented 1 year ago

@mherman09, to compile motorcycle-ap-ratestate-bath-serial, the code that takes into account temperature effects, after you compiled motorcycle-ap-ratestate-serial, use

make clean
make motorcycle-ap-ratestate-bath-serial

Otherwise, the compiled objects will be missing several key variables. I added relevant instructions in INSTALL.md.

Thank you again for your feedback. Let me know if there are further issues or suggestions.

mherman09 commented 1 year ago

Great! That works. Just so you know, the new INSTALL.md says:

The binary file will be located in `motorcycle/2d/antiplane/build/motorcycle-ap-ratestate-serial`. The `-serial` suffix indicates serial or shared-memory parallelism with *OpenMP*. To compile the code that includes thermal effects in the same directory, use

    make clean
    make motorcycle-ap-ratestate-serial

But I think the second instruction should be make motorcycle-ap-ratestate-bath-serial, right?

Also, is there a particular reason why the two executables (with and without thermal effects) should not both be able to be built and available for a user?

sbarbot commented 1 year ago

Correct. This should be make motorcycle-ap-ratestate-bath-serial. I updated the README to include the codes with temperature effects

I also added instructions to compile motorcycle-3d-ratestate-bath in INSTALL.md.

Thank you again for checking everything.

mherman09 commented 1 year ago

Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?

The high-level functionality of motorcycle is concisely described. Its purpose is clear to me as a person who thinks about earthquake cycles and geodynamics problems. I am not sure that the purpose is clear for a "diverse, non-specialist audience," but I think this might require too much geophysics/engineering background for this type of paper. I personally think the Summary is nicely written, so I am going to check the item off the list.

mherman09 commented 1 year ago

@sbarbot @jedbrown I believe my review is finished. I think the Motorcycle package fulfills all the items on the checklist! Thank you for thinking of me as a reviewer, and again I apologize for the delay.

jedbrown commented 1 year ago

Thanks, @mherman09.

@thehalfspace :wave: Just checking if there's anything you need for your review?

jedbrown commented 1 year ago

@thehalfspace :wave: Can you let us know when you'll be able to complete your review?

thehalfspace commented 1 year ago

Hi Dr. Jed,

Sorry for the delay. I got booted from my work and had to spend some time looking for work so as to maintain my visa status, hence the delay.

I'll have the review sent out within a week.

Sincerely, — Prithvi Thakur Graduate Student University of Michigan Personal Website

Sent with Proton Mail secure email.

------- Original Message ------- On Saturday, August 12th, 2023 at 10:31 AM, Jed Brown @.***> wrote:

@.***(https://github.com/thehalfspace) 👋 Can you let us know when you'll be able to complete your review?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

jedbrown commented 1 year ago

Uff, sounds rough. I'm glad that's resolved, and thanks for the update.

thehalfspace commented 1 year ago

Review checklist for @thehalfspace

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

thehalfspace commented 12 months ago

I have tested the motorcycle software and its core functionalities on my local machine. The package is excellent with very well written documentation, and the tests are described appropriately. The comparison with other softwares are appropriately referenced via SCEC benchmark suites, and the applications of this software is also appropriately referenced in the README. I had to browse through the other comments from @mherman09 and @sbarbot to compile motorcycle-ap-ratestate-bath-serial, but I believe they are sufficiently documented in the INSTALL.md file now. I do not have any major comments on the application and I recommend publishing it. I am only pointing out a few minor comments below and some suggestions for future work:

  1. I believe it would be good to reference some of the example files (run.sh) early in the documentation as a section on parameter setup. It was easy for me to follow but I believe for someone with less experience in seismic cycle simulations, it would make it easier to start with as a jumping-off point, and the end user would not have to browse through a list of files to run a test simulation.
  2. It would be nice to hyperlink the sources for netcdf, netcdf-f, openmpi, and fftw packages in INSTALL.md.
  3. For future work, I'd suggest creating a containerized application (e.g., using docker) for easier cross-platform compatibility and shipping software to users on different machines. Alternatively, a python-based conda virtual environment can be set up easily with specified list of dependencies.

I do not have more comments, and I apologize for the delay in sending my review.

-Prithvi

sbarbot commented 12 months ago

@thehalfspace,

Thank you for your comments. Following additional comments from @jedbrown on another software, I will also include a global procedure for compilation and installation using CMake. I have to learn more about it, so it will take a bit of time.

Regarding point 1, do you suggest I provide a functional input file embedded in the documentation? I had thought of it, but a functioning, meaningful input file is very long, requiring many fault patches, making it less practical than a standalone bash file in the examples/tutorials directory. Or do you suggest I add a link to a file in the documentation? Or I could add a command-line option to dump a functioning input file to the standard output. What do you recommend more specifically?

Point 2 is clear.

Point 3 is a great suggestion. Can docker be used with mpirun for the 3d case?

thehalfspace commented 12 months ago

I was just suggesting adding a link/reference to examples/tutorial directory somewhere near the top of the documentation or in README. Yes, I don't think adding a functional input file would be very useful as each case would require a separate file and would be needed to updated constantly as more features get added, which is a source for more bugs.

Point 3 is more of a recommendation for future work. I have myself not run docker with mpirun yet, but I do know that it is not very trivial to implement, especially with fortran libraries. I know it can be done though (for example, look at Fenics or Firedrake packages). I think conda virtual environment with a docker container might be the way to go?

jedbrown commented 12 months ago

Containers work fine. I've mostly used Singularity and Shifter, both of which have the ability to use the host MPI (which might use a vendor network). One anti-pattern is that people realize that when releasing software inside a container, they only need to build in one environment, and then the software is never tested or built elsewhere, which usually leads to maintenance problems.

sbarbot commented 12 months ago

@thehalfspace I have created an online document that links to the repository at https://motorcycle.readthedocs.io/. The online documentation includes example input files. I have added pointers to the tutorial examples in README.md.

I have added hyperlinks to netcdf, netcdf-f, openmpi, and fftw in INSTALL.md.

I am still working on streamlining the configuration and installation.

sbarbot commented 11 months ago

@jedbrown I have added a cmake installation for all the binary files of the package. The installation is tested with a bitbucket pipeline. I believe this was the remaining task for this project. Are you ready to accept the paper for publication?

jedbrown commented 11 months ago

This isn't a blocker, but running make -j (or if -j flags are in MAKEFLAGS), the build fails due to errors like this.

f951: Fatal Error: Cannot rename module file ‘exportnetcdf.mod0’ to ‘exportnetcdf.mod’: No such file or directory
compilation terminated.
Error copying Fortran module "2d/antiplane/src-serial/exportnetcdf.mod".  Tried "2d/antiplane/src-serial/EXPORTNETCDF.mod" and "2d/antiplane/src-serial/exportnetcdf.mod".
Error copying Fortran module "3d/src/exportnetcdf.mod".  Tried "3d/src/EXPORTNETCDF.mod" and "3d/src/exportnetcdf.mod".

This is usually caused by incorrectly-specified dependencies. If I use cmake -G Ninja .. and then ninja, I get

[33/140] Generating Fortran dyndep file 2d/antiplane/src-serial/CMakeFiles/motorcycle-ap-ratestate-cz-serial.dir/Fortran.dd
ninja: build stopped: multiple rules generate 2d/antiplane/src-serial/types.mod.

This suggests that make -j1 (serial) succeeding is probably non-deterministic.

sbarbot commented 11 months ago

The code is not amenable to fully parallel builds. Only the different subdirectories defined in ADD_SUBDIRECTORY should be built in parallel, but CMake attempts to build all the ADD_EXECUTABLE commands in parallel. The only apparent solution is to use make -j1.

There is another, more pressing issue. CMake does not allow multithreaded fftw with OpenMP. CMake only inserts the -lfftw3_omp library, when both -lfftw3_omp and -lfftw3 should be set. As a result, we can compile and eventually run the code with CMake without OpenMP parallelism, but the performance is not optimal. This is a CMake bug.

sbarbot commented 11 months ago

@jedbrown I have fixed the issue with OpenMP. Unless you have other concerns or suggestions, I believe this is ready for publication.

jedbrown commented 10 months ago

Sorry about the disruption. For the parallel build, I think it's because the same file names are used for those modules and CMake evidently doesn't know how to use -J to control the module output directory. If you made sure that the modules have unique names (either by avoiding copies or by giving them different names), I think the parallel build would work. (Fortran module names are effectively global and to my knowledge, the language doesn't offer namespacing so CMake does something plausible and in signature style, doesn't check for collisions.) This is not a blocker, but it either needs to be fixed or documented.

On the second issue, I can fix the compile failure when USE_OPENMP=0 with this:

diff --git i/cmake/FindFFTW.cmake w/cmake/FindFFTW.cmake
index b042aa8..44730b6 100644
--- i/cmake/FindFFTW.cmake
+++ w/cmake/FindFFTW.cmake
@@ -14,8 +14,10 @@ ENDIF()
 FIND_PATH(FFTW_INCLUDES fftw3.h)

 FIND_LIBRARY(FFTW_LIBRARIES NAMES fftw3)
+set(FFTW_REQUIRED FFTW_LIBRARIES)
 IF (USE_OPENMP)
   FIND_LIBRARY(FFTW_OMP_LIBRARIES NAMES fftw3_omp)
+  list(PREPEND FFTW_REQUIRED FFTW_OMP_LIBRARIES)
 ENDIF()

 MESSAGE("-- FFTW library: " ${FFTW_LIBRARIES} " " ${FFTW_OMP_LIBRARIES})
@@ -23,6 +25,6 @@ MESSAGE("-- FFTW library: " ${FFTW_LIBRARIES} " " ${FFTW_OMP_LIBRARIES})
 # handle the QUIETLY and REQUIRED arguments and set FFTW_FOUND to TRUE if
 # all listed variables are TRUE
 INCLUDE(FindPackageHandleStandardArgs)
-FIND_PACKAGE_HANDLE_STANDARD_ARGS(FFTW DEFAULT_MSG FFTW_LIBRARIES FFTW_OMP_LIBRARIES FFTW_INCLUDES)
+FIND_PACKAGE_HANDLE_STANDARD_ARGS(FFTW DEFAULT_MSG ${FFTW_REQUIRED} FFTW_INCLUDES)

 MARK_AS_ADVANCED(FFTW_LIBRARIES FFTW_OMP_LIBRARIES FFTW_INCLUDES)

And then it links both when USE_OPENMP=1 and only the serial otherwise. When both are linked, I see this, which looks right except the link order should be reversed -- you can change by reversing the order where you currently have ${FFTW_LIBRARIES} ${FFTW_OMP_LIBRARIES}.

$ readelf -d 2d/antiplane/build/motorcycle-ap-ratestate-cz-serial

Dynamic section at offset 0x1fd60 contains 34 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libnetcdff.so.7]
 0x0000000000000001 (NEEDED)             Shared library: [libnetcdf.so.19]
 0x0000000000000001 (NEEDED)             Shared library: [libfftw3.so.3]
 0x0000000000000001 (NEEDED)             Shared library: [libfftw3_omp.so.3]
 0x0000000000000001 (NEEDED)             Shared library: [libgfortran.so.5]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgomp.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
[...]
sbarbot commented 10 months ago

@jedbrown Thank you for the suggestions. I can now run cmake with -DUSE_OPENMP=1 and -DUSE_OPENMP=0. I also switched the order of the libraries libfftw3_omp and libfftw3.

Running otool, I get the following output:

$ otool -L 2d/antiplane/build/motorcycle-ap-ratestate-cz-serial 2d/antiplane/build/motorcycle-ap-ratestate-cz-serial: /usr/local/opt/netcdf-fortran/lib/libnetcdff.7.dylib (compatibility version 7.0.0, current version 7.1.0) /usr/local/opt/netcdf/lib/libnetcdf.19.dylib (compatibility version 19.0.0, current version 19.0.0) /usr/local/opt/fftw/lib/libfftw3_omp.3.dylib (compatibility version 10.0.0, current version 10.10.0) /usr/local/opt/fftw/lib/libfftw3.3.dylib (compatibility version 10.0.0, current version 10.10.0) /usr/local/opt/gcc/lib/gcc/current/libgfortran.5.dylib (compatibility version 6.0.0, current version 6.0.0) /usr/local/opt/gcc/lib/gcc/current/libgomp.1.dylib (compatibility version 2.0.0, current version 2.0.0) /usr/local/opt/gcc/lib/gcc/current/libgcc_s.1.1.dylib (compatibility version 1.0.0, current version 1.1.0) /usr/local/opt/gcc/lib/gcc/current/libquadmath.0.dylib (compatibility version 1.0.0, current version 1.0.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.100.5)

jedbrown commented 9 months ago

Sounds good. Would you mind making the module names distinct? (Part of the issue is that many build tools are parallel by default (or users have MAKEFLAGS=-j8) and the present configuration is outright rejected by the build tools many would prefer, such as Ninja. I think it's also a vulnerability/brittleness that could arise in a future CMake version since its Fortran generator interprets module names as global.)

$ cmake -G Ninja ..
[...]
$ ninja
[30/140] Generating Fortran dyndep file 2d/antiplane/src-serial/CMakeFiles/motorcycle-ap-ratestate-cz-serial.dir/Fortran.dd
ninja: build stopped: multiple rules generate 2d/antiplane/src-serial/types.mod.
sbarbot commented 9 months ago

I'm not sure how to operate. It looks like all binary files need their separate versions of the module files. I may need to separate each version of the binary file into a designated folder where a separate copy of the module files can be found. That seems suboptimal to me, as I would need to maintain several copies of the same file. I already do this to some extent for the codes in 2d/antiplane, 2d/planestrain, and 3d for modules like exportnetcdf and types, but now I would need to do this for even more versions of the code. Do you have a better idea of how to proceed? I can see the appeal of using Ninja. It is very fast.