Add max_model_depth variable to cartesian and spherical geometries #367

MFraters · 2021-11-08T20:23:47Z

This is a first step based on the discussion in both #331 and #345. This only add max_model_depth variable to cartesian and spherical geometries with a get function and a radius input parameter for the spherical geometry model to define the max_model_depth.

Merging this will allow to start writing code which uses this internally and within ASPECT.

codecov · 2021-11-08T20:26:15Z

Codecov Report

Merging #367 (eec0273) into main (37146a4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #367   +/-   ##
=======================================
  Coverage   98.88%   98.88%           
=======================================
  Files          77       77           
  Lines        4560     4578   +18     
=======================================
+ Hits         4509     4527   +18     
  Misses         51       51

Impacted Files	Coverage Δ
source/coordinate_systems/cartesian.cc	`100.00% <100.00%> (ø)`
source/coordinate_systems/spherical.cc	`100.00% <100.00%> (ø)`
source/parameters.cc	`99.59% <100.00%> (+<0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37146a4...eec0273. Read the comment docs.

github-actions · 2021-11-08T20:44:25Z

Benchmark	Main	Feature	Difference (99.9% CI)
Slab interpolation simple none	1.405 ± 0.056 (s=328)	1.397 ± 0.051 (s=317)	-1.5% .. +0.5%
Slab interpolation curved simple none	1.226 ± 0.003 (s=415)	1.225 ± 0.003 (s=322)	-0.1% .. -0.0%
Spherical slab interpolation simple none	1.542 ± 0.034 (s=286)	1.539 ± 0.036 (s=301)	-0.8% .. +0.4%
Slab interpolation simple curved CMS	1.962 ± 0.057 (s=225)	1.951 ± 0.060 (s=237)	-1.5% .. +0.4%
Spherical slab interpolation simple CMS	2.592 ± 0.075 (s=164)	2.592 ± 0.068 (s=186)	-1.0% .. +1.0%
Spherical fault interpolation simple none	1.525 ± 0.038 (s=294)	1.521 ± 0.038 (s=300)	-0.9% .. +0.4%

…th a get function and a radius input parameter for the spherical geometry model to define the max_model_depth.

source/coordinate_systems/cartesian.cc

bangerth · 2021-11-09T04:51:59Z

source/parameters.cc

+                size_t size = Pointer((base_path + "/oneOf").c_str()).Get(declarations)->Size();
+#ifdef debug
+                bool found = false;
+#endif


This has nothing to do with the patch itself, but just as a comment: I assume that you are putting the #ifdef around this statement because you would otherwise get a warning about the found variable only ever being written to, but never read, in release mode? If so, you can avoid the warning if you put

(void)found;

in front of the WBAssert. This is interpreted as a "read" of the variable, but you are just discarding the value so read.

interesting, I will try that!

on my laptop those if statement do not seem to matter, but probably for one of the tester configurations it does. I will look into it in a different pull request.

bangerth · 2021-11-09T04:52:51Z

tests/unit_tests/unit_test_world_builder.cc

@@ -3823,9 +3823,12 @@ TEST_CASE("WorldBuilder Parameters")
 {
  // First test a world builder file with a cross section defined
  std::string file = WorldBuilder::Data::WORLD_BUILDER_SOURCE_DIR + "/tests/data/type_data.json";
-  std::string file_name = WorldBuilder::Data::WORLD_BUILDER_SOURCE_DIR + "/tests/data/subducting_plate_different_angles_spherical.wb";
+  std::string file_name = WorldBuilder::Data::WORLD_BUILDER_SOURCE_DIR + "/tests/data/subducting_plate_different_angles_cartesian.wb";


Aren't you abandoning the old test here?

I noticed the only thing happening with file_name here is WorldBuilder::World world(file_name); and then Parameters prm(world);. Then the prm is actually initialized with a different file. These first two steps on this exact file are already being tested in line 7099-7100, so there is no reason to repeat that here. And there was no place in the unit tester which did this for the Cartesian case, so I think it is a nice expansion of the testing without losing anything.

I could have made a new test as well, but I guess I was a bit lazy ;)

MFraters

Thanks for the review!

source/coordinate_systems/cartesian.cc

MFraters · 2021-11-09T07:50:24Z

source/parameters.cc

+                size_t size = Pointer((base_path + "/oneOf").c_str()).Get(declarations)->Size();
+#ifdef debug
+                bool found = false;
+#endif


interesting, I will try that!

MFraters · 2021-11-09T07:57:48Z

tests/unit_tests/unit_test_world_builder.cc

@@ -3823,9 +3823,12 @@ TEST_CASE("WorldBuilder Parameters")
 {
  // First test a world builder file with a cross section defined
  std::string file = WorldBuilder::Data::WORLD_BUILDER_SOURCE_DIR + "/tests/data/type_data.json";
-  std::string file_name = WorldBuilder::Data::WORLD_BUILDER_SOURCE_DIR + "/tests/data/subducting_plate_different_angles_spherical.wb";
+  std::string file_name = WorldBuilder::Data::WORLD_BUILDER_SOURCE_DIR + "/tests/data/subducting_plate_different_angles_cartesian.wb";


I noticed the only thing happening with file_name here is WorldBuilder::World world(file_name); and then Parameters prm(world);. Then the prm is actually initialized with a different file. These first two steps on this exact file are already being tested in line 7099-7100, so there is no reason to repeat that here. And there was no place in the unit tester which did this for the Cartesian case, so I think it is a nice expansion of the testing without losing anything.

I could have made a new test as well, but I guess I was a bit lazy ;)

MFraters · 2021-11-10T23:05:28Z

@bangerth I think that all your comments have been addressed. I will wait with merging this for few hours in case you have more comments.

bangerth · 2021-11-11T04:58:30Z

I won't get to it before tomorrow afternoon, but feel free to go ahead.

MFraters · 2021-11-11T07:41:59Z

I am happy to wait if you want to have another look at it :)

bangerth

I have no further comments about the patch, so go ahead with merging! I guess the next step will be to propagate this information to the feature plugins?

MFraters · 2021-11-12T01:36:57Z

Great, thanks! With this pull request the information should now be available anywhere in the world builder, so it will be both using it and making the changes to the interface which now have become possible.

bangerth · 2021-11-13T05:08:10Z

Sounds good. Feel free to tag me in any follow-up patches for which you are looking for input!

MFraters mentioned this pull request Nov 8, 2021

Define model geometry extend in or to the world builder. #345

Closed

Allow for correct processing of default values in coordinate system.

6b24038

MFraters force-pushed the add_coordinate_system_max_model_depth branch from 8e3a09d to 418bc88 Compare November 8, 2021 22:12

MFraters added 3 commits November 8, 2021 15:00

Add max_model_depth variable to cartesian and spherical geometries wi…

904ac21

…th a get function and a radius input parameter for the spherical geometry model to define the max_model_depth.

add unit test for max_model_depth

bd65c91

add changelog entry for new max model depth parameter.

eec0273

MFraters force-pushed the add_coordinate_system_max_model_depth branch from 4d91dd5 to eec0273 Compare November 8, 2021 23:02

MFraters added the ready to merge Pull request is ready to merge. May be waiting for tests to complete or other reviews. label Nov 8, 2021

bangerth reviewed Nov 9, 2021

View reviewed changes

MFraters commented Nov 9, 2021

View reviewed changes

MFraters mentioned this pull request Nov 11, 2021

Expand coordinate system documentation #371

Merged

bangerth reviewed Nov 11, 2021

View reviewed changes

MFraters merged commit 9dc1c89 into GeodynamicWorldBuilder:main Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add max_model_depth variable to cartesian and spherical geometries #367

Add max_model_depth variable to cartesian and spherical geometries #367

MFraters commented Nov 8, 2021

codecov bot commented Nov 8, 2021 •

edited

Loading

github-actions bot commented Nov 8, 2021 •

edited

Loading

bangerth Nov 9, 2021

MFraters Nov 9, 2021

MFraters Nov 9, 2021

bangerth Nov 9, 2021

MFraters Nov 9, 2021

MFraters left a comment

MFraters Nov 9, 2021

MFraters Nov 9, 2021

MFraters commented Nov 10, 2021

bangerth commented Nov 11, 2021

MFraters commented Nov 11, 2021

bangerth left a comment

MFraters commented Nov 12, 2021

bangerth commented Nov 13, 2021

Add max_model_depth variable to cartesian and spherical geometries #367

Add max_model_depth variable to cartesian and spherical geometries #367

Conversation

MFraters commented Nov 8, 2021

codecov bot commented Nov 8, 2021 • edited Loading

Codecov Report

github-actions bot commented Nov 8, 2021 • edited Loading

bangerth Nov 9, 2021

Choose a reason for hiding this comment

MFraters Nov 9, 2021

Choose a reason for hiding this comment

MFraters Nov 9, 2021

Choose a reason for hiding this comment

bangerth Nov 9, 2021

Choose a reason for hiding this comment

MFraters Nov 9, 2021

Choose a reason for hiding this comment

MFraters left a comment

Choose a reason for hiding this comment

MFraters Nov 9, 2021

Choose a reason for hiding this comment

MFraters Nov 9, 2021

Choose a reason for hiding this comment

MFraters commented Nov 10, 2021

bangerth commented Nov 11, 2021

MFraters commented Nov 11, 2021

bangerth left a comment

Choose a reason for hiding this comment

MFraters commented Nov 12, 2021

bangerth commented Nov 13, 2021

codecov bot commented Nov 8, 2021 •

edited

Loading

github-actions bot commented Nov 8, 2021 •

edited

Loading