Skip to content

Set bedrock diffusivity in tests to something very small instead of -1#6997

Merged
gassmoeller merged 1 commit into
geodynamics:mainfrom
anne-glerum:fix_bedrock_diff_tests
Jun 23, 2026
Merged

Set bedrock diffusivity in tests to something very small instead of -1#6997
gassmoeller merged 1 commit into
geodynamics:mainfrom
anne-glerum:fix_bedrock_diff_tests

Conversation

@anne-glerum

@anne-glerum anne-glerum commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

A bedrock diffusivity of -1 does not make sense. However, for sediment diffusivity a value of -1 is allowed, as it indicates to FastScape that the same diffusivity should be used for the sediment as for the bedrock.

I will update the test output files once I've run them locally in docker after #6996, #6994 and #6995 have been merged.

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.

@gassmoeller gassmoeller left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The other PRs are merged. Is this ready to be merged?

end
set Sediment river incision rate = 1e-5
set Bedrock diffusivity = -1
set Bedrock diffusivity = 1e-10

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a a meaning behind 1e-10? And I assume 3e-18 in the other test is just the same value converted to seconds?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is no special meaning, I just wanted to pick something practically zero, but not zero, in case that triggers something on FastScape's side.

Yes, 3e-18 is the same but in seconds.

@anne-glerum

Copy link
Copy Markdown
Contributor Author

Is this ready to be merged?

Before fixing the bugs in #6963, I couldn't produce the updated test output, so this PR doesn't have updated test output (I don't actually know whether the change affects the results). But perhaps it is better to first merge #7001, point the tester to my branch, then merge #7002, and then update this PR?

@anne-glerum

Copy link
Copy Markdown
Contributor Author

/rebuild

@gassmoeller gassmoeller changed the title [WIP] Set bedrock diffusivity in tests to something very small instead of -1 Set bedrock diffusivity in tests to something very small instead of -1 Jun 23, 2026

@gassmoeller gassmoeller left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The failing test is the same one as in #7011 and unrelated. This looks good to go, do you think it is ready as well?

@anne-glerum

Copy link
Copy Markdown
Contributor Author

Yes, I just checked with the docker image, and this PR doesn't change the model output (except for Fastscape vtk output).

I do have one question related to this: Fastscape produces vtk output and this is included as reference files for the Fastscape tests. Although vtk output is not compared by the tester, the vtk binary output is different every time I run a test. Is it important to keep this vtk output consistent? Can we just remove it from the tests as it's not used?

@gassmoeller gassmoeller left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, lets merge this PR then.

Generally, we try to avoid binary outputs, since it is not useful to compare. As long as the tests show somehow without the vtk output that their results have not changed since their introduction (e.g statistics or screen output) then the vtk files should be removed. I am actually a bit surprised the tester doesnt notice the change. I remember that it did regularly complain about solution.vtu files in the past.

@gassmoeller gassmoeller merged commit d40a376 into geodynamics:main Jun 23, 2026
8 of 9 checks passed
@anne-glerum

Copy link
Copy Markdown
Contributor Author

Generally, we try to avoid binary outputs, since it is not useful to compare. As long as the tests show somehow without the vtk output that their results have not changed since their introduction (e.g statistics or screen output) then the vtk files should be removed. I am actually a bit surprised the tester doesnt notice the change. I remember that it did regularly complain about solution.vtu files in the past.

What I saw with a different PR and here with testing in the docker image is that:

  1. Running ASPECT_GENERATE_REFERENCE_OUTPUT=1 ctest -R fastscape -V changes the vtk output
  2. Running ctest -R fastscape does fail on the vtk output
  3. Using the vkt output generated with ASPECT_GENERATE_REFERENCE_OUTPUT=1 to then run normal ctest again does not make the tests succeed. I.e. the vtk's are different every time.

The dealii-master tester will not fail, because it doesn't compare test output, right? Should we include Fastscape in the other images?

The FastScape vtk output could be of higher resolution, and they contain other data, e.g. drainage area and total erosion, than what ASPECT knows about. The most important, I think, is the resulting topography in ASPECT, which we can write out instead.

@anne-glerum anne-glerum deleted the fix_bedrock_diff_tests branch June 24, 2026 13:38
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.

2 participants