Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vankeken 2008 subduction benchmark #5259

Merged

Conversation

danieldouglas92
Copy link
Contributor

@danieldouglas92 danieldouglas92 commented Jul 11, 2023

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.

Describe what you did in this PR and why you did it.

Before your first pull request:

For all pull requests:

For new features/models or changes of existing features:

  • I have tested my new feature locally to ensure it is correct.
  • I have created a testcase for the new feature/benchmark in the tests/ directory.
  • I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.

@danieldouglas92 danieldouglas92 changed the title Vankeken 2008 subdction benchmark Vankeken 2008 subduction benchmark Jul 11, 2023
Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments already. I'm going to look through the .cc files on occasion.

cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/plugin/van_Keken_mesh.cc Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
Copy link
Contributor

@naliboff naliboff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieldouglas92 - Thanks for contributing! A few general questions, comments, and requests in addition to specific comments on some files:

  1. It looks like many of the functions in the plugin, such as those related to pressure, are not used. I would recommend removing them and downsizing the plugin to just what is needed. It may be that you could just use the cookbook plugin that already exists in the code for prescribing velocities?
  2. Can you adjust the paraview images such that (i) the background is white, (ii) the colorbar and colorbar text are larger, and (iii) add in a scale bar for each axis?
  3. At some point during the review process, please add a test that run a short, low resolution version of this model.

Thanks!

@naliboff
Copy link
Contributor

@maxrudolph - This PR is a follow-up to your van Keken 2008 benchmark PR (#1864) by @danieldouglas92, @cedrict and @bangerth. Creating a new mesh that aligns with the edges of the slab seems to have resolved many of the issues seen in the original PR. At some point, would you be willing to take a look and do a review?

Copy link
Contributor

@naliboff naliboff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieldouglas92 - Thanks for the updates and indeed the PR is almost there.

A few minor comments here and there, in addition to the following request - for the test case, can you link to the cookbook prm in the following manner, and then just modify the relevant parameters?
include $ASPECT_SOURCE_DIR/cookbooks/vankeken_subduction/vankeken_corner_flow.prm

This will ensure the cookbook PRM itself is able to run with the current main branch.

Thanks!

cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/doc/vankeken_subduction.md Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/plugin/van_Keken_mesh.cc Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a changelog entry to doc/modules/...?

cookbooks/vankeken_subduction/plugin/van_Keken_mesh.cc Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/plugin/van_Keken_mesh.cc Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/plugin/van_Keken_mesh.cc Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
cookbooks/vankeken_subduction/vankeken_corner_flow.prm Outdated Show resolved Hide resolved
doc/sphinx/references.bib Outdated Show resolved Hide resolved
@naliboff
Copy link
Contributor

/rebuild

Copy link
Contributor

@naliboff naliboff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieldouglas92 - Good to go from my side once the testers have finished and @bangerth gives a thumbs up as well. Thanks for the contribution!

@gassmoeller
Copy link
Member

Tests are failing

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thumbs up, but as noted above, the testers are failing.

@bangerth
Copy link
Contributor

Specifically, the failure is in the new tests:

-----------------------------------------------------------------------------
-- This is ASPECT, the Advanced Solver for Problems in Earth's ConvecTion.
--     . version 2.6.0-pre
--     . using deal.II 9.6.0-pre (master, 0847ac00f9)
--     .       with 32 bit indices and vectorization level 1 (128 bits)
--     . using Trilinos 13.2.0
--     . using p4est 2.3.2
--     . running in DEBUG mode
--     . running with 1 MPI process
-----------------------------------------------------------------------------

Loading shared library <./libvankeken_subduction.debug.so>

[d6ac324d933f:30195] *** Process received signal ***
[d6ac324d933f:30195] Signal: Floating point exception (8)
[d6ac324d933f:30195] Signal code: Floating point divide-by-zero (3)
[d6ac324d933f:30195] Failing at address: 0x5637129fcc08
[d6ac324d933f:30195] [ 0] /lib/x86_64-linux-gnu/libpthread.so.0(+0x14420)[0x7fe92f506420]
[d6ac324d933f:30195] [ 1] /__w/aspect/aspect/build/aspect(_ZN6aspect19AdiabaticConditions14ComputeProfileILi2EE10initializeEv+0x898)[0x5637129fcc08]
[d6ac324d933f:30195] [ 2] /__w/aspect/aspect/build/aspect(_ZN6aspect9SimulatorILi2EEC1EP19ompi_communicator_tRN6dealii16ParameterHandlerE+0x15d5)[0x563711a77dc5]
[d6ac324d933f:30195] [ 3] /__w/aspect/aspect/build/aspect(_Z13run_simulatorILi2EEvRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES7_bbb+0x13b)[0x5637128a6f4b]
[d6ac324d933f:30195] [ 4] /__w/aspect/aspect/build/aspect(main+0x5aa)[0x5637119f67ba]
[d6ac324d933f:30195] [ 5] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7fe92f324083]
[d6ac324d933f:30195] [ 6] /__w/aspect/aspect/build/aspect(_start+0x2e)[0x5637119fff4e]
[d6ac324d933f:30195] *** End of error message ***

@maxrudolph
Copy link
Contributor

Hi @danieldouglas92 it's great to see that you got a more reasonable solution to the van Keken 2008 corner flow benchmark. I was looking at the pull request and I can't find the actual benchmark values. Do you compute the point values and average values for wedge temperature that are used for the inter-code comparison in the paper?

@danieldouglas92
Copy link
Contributor Author

Hey @maxrudolph, for the purposes of this PR I just wanted to get this into ASPECT as a cookbook, but I have not tried to compare the output to the 2008 benchmark yet. That is something that I would be interesting in doing at some point, but because it isn't done yet Im putting this in the cookbooks instead of benchmarks directory.

@bangerth
Copy link
Contributor

Still same error:

Loading shared library <./libvankeken_subduction.debug.so>

[37aefc73f38c:30196] *** Process received signal ***
[37aefc73f38c:30196] Signal: Floating point exception (8)
[37aefc73f38c:30196] Signal code: Floating point divide-by-zero (3)
[37aefc73f38c:30196] Failing at address: 0x55bc0347dc08
[37aefc73f38c:30196] [ 0] /lib/x86_64-linux-gnu/libpthread.so.0(+0x14420)[0x7f180bfea420]
[37aefc73f38c:30196] [ 1] /__w/aspect/aspect/build/aspect(_ZN6aspect19AdiabaticConditions14ComputeProfileILi2EE10initializeEv+0x898)[0x55bc0347dc08]
[37aefc73f38c:30196] [ 2] /__w/aspect/aspect/build/aspect(_ZN6aspect9SimulatorILi2EEC1EP19ompi_communicator_tRN6dealii16ParameterHandlerE+0x15d5)[0x55bc024d1235]
[37aefc73f38c:30196] [ 3] /__w/aspect/aspect/build/aspect(_Z13run_simulatorILi2EEvRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES7_bbb+0x13b)[0x55bc03328a6b]
[37aefc73f38c:30196] [ 4] /__w/aspect/aspect/build/aspect(main+0x5aa)[0x55bc02444fca]
[37aefc73f38c:30196] [ 5] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7f180be08083]
[37aefc73f38c:30196] [ 6] /__w/aspect/aspect/build/aspect(_start+0x2e)[0x55bc0244e75e]
[37aefc73f38c:30196] *** End of error message ***
ninja: build stopped: subcommand failed.

What happens if you run this test (using ctest) locally on your machine?

@danieldouglas92 danieldouglas92 force-pushed the vankeken_2008_subdction_benchmark branch from bdb1b22 to f0ffb6f Compare July 14, 2023 05:38
@@ -0,0 +1,14 @@

include $ASPECT_SOURCE_DIR/cookbooks/vankeken_subduction/vankeken_corner_flow.prm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the failing test is here. In this test, you are including the .prm file from the cookbooks directory, which contains the following lines:

# Load the prescribed velocity and pressure library
set Additional shared libraries            = ./plugin/libvankeken_subduction.so

But in the directory where the test is executed, there is no such shared library and the test consequently aborts. I'm surprised that the test works for you locally, if you run it with the commands

make setup_tests
ctest -R vankeken

in your build directory.

This kind of test design doesn't work because it assumes that the shared library you want is located in a directory specified in the cookbook, not in the test. Are there other tests that reference .prm files from the cookbooks where we could look up how this is handled there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a .cc with the same name in the test directory and use an include statement of the source file in the cookbooks directory.

@danieldouglas92 danieldouglas92 force-pushed the vankeken_2008_subdction_benchmark branch from b9a7bc3 to beb7eee Compare July 14, 2023 17:18
@maxrudolph
Copy link
Contributor

@danieldouglas92 In my very old PR on this benchmark, I have some python code to do the benchmark comparison. It is here... https://github.com/maxrudolph/aspect/tree/vankeken_subduction
I'm happy to help with a follow up PR or add to yours if you'd like.

@gassmoeller
Copy link
Member

We are finalizing this years hackathon so we would prefer to merge this in the current state today, but follow up PRs with more accurate comparisons are very welcome @danieldouglas92 and @maxrudolph.

@gassmoeller gassmoeller merged commit 6b21e0c into geodynamics:main Jul 14, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants