Added coordinate systems to Gravity model function plug-in #5752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gassmoeller
requested changes
Jun 3, 2024
| @@ -0,0 +1,3 @@ | |||
| Added: The Function expression plug-in for the Gravity model can now evaluate functions in multiple coordinate systems. The plug-in was ammended to read in entries to the "Coordinate system" parameter in the Function subsection and Gravity Model section of the parameter file. There are two added tests for the Gravity model function plug-in to ensure the cartesian and spherical functions evaluate as expected in a 2D cylindrical annulus. | |||
There was a problem hiding this comment.
Suggested change
| Added: The Function expression plug-in for the Gravity model can now evaluate functions in multiple coordinate systems. The plug-in was ammended to read in entries to the "Coordinate system" parameter in the Function subsection and Gravity Model section of the parameter file. There are two added tests for the Gravity model function plug-in to ensure the cartesian and spherical functions evaluate as expected in a 2D cylindrical annulus. | |
| Added: The Function expression plug-in for the Gravity model can now evaluate functions in multiple coordinate systems. The plug-in was amended to read in entries to the "Coordinate system" parameter in the Function subsection and Gravity Model section of the parameter file. There are two added tests for the Gravity model function plug-in to ensure the cartesian and spherical functions evaluate as expected in a 2D cylindrical annulus. |
What about a test for the depth option?
source/gravity_model/function.cc
Outdated
Comment on lines
44
to
45
| for (unsigned int d=0; d<dim; ++d) | ||
| gravity[d] = function.value(position,d); | ||
| gravity[d] = function.value(Utilities::convert_array_to_point<dim>(point.get_coordinates()),d); |
There was a problem hiding this comment.
Your code is not wrong, but this one will be slightly faster, because some computations will be done only once instead of dim times:
const Point<dim> point_in_coordinate_system = Utilities::convert_array_to_point<dim>(point.get_coordinates());
for (unsigned int d=0; d<dim; ++d)
gravity[d] = function.value(point_in_coordinates,d);
source/gravity_model/function.cc
Outdated
Comment on lines
71
to
77
| /** | ||
| * Choose the coordinates to evaluate the maximum refinement level | ||
| * function. The function can be declared in dependence of depth, | ||
| * cartesian coordinates or spherical coordinates. Note that the order | ||
| * of spherical coordinates is r,phi,theta and not r,theta,phi, since | ||
| * this allows for dimension independent expressions. | ||
| */ |
There was a problem hiding this comment.
Remove all this. The declare_entry function already receives a lot of documentation about what this parameter means, so this comment is duplicating information.
tests/gravity_model_cartesian.prm
Outdated
Comment on lines
1
to
5
| # This setup is a copy of the | ||
| # benchmarks/onset-of-convection/convection-box-base.prm | ||
| # contributed by Max Rudolph, with the difference that | ||
| # parameter values are specified explicitly in the input file | ||
| # (rather than through an ipython notebook). |
There was a problem hiding this comment.
I dont think the detailed reference to the benchmark cookbook here is necessary. You have modified this file quite a bit and it serves a different purpose. It would be more useful to describe the purpose of this test, e.g. something like:
This is a test based on benchmarks/onset-of-convection/convection-box-base.prm. It makes sure that the gravity plugin 'function' works correctly for a Cartesian coordinate system.
tests/gravity_model_spherical.prm
Outdated
| @@ -0,0 +1,100 @@ | |||
| # This setup is a copy of the | |||
There was a problem hiding this comment.
Same comment as for the other test.
gassmoeller
reviewed
Jun 5, 2024
| * The coordinate representation to evaluate the function. Possible | ||
| * choices are depth, cartesian and spherical. | ||
| */ | ||
| Utilities::Coordinates::CoordinateSystem coordinate_system; |
There was a problem hiding this comment.
I think you need to add #include <aspect/utilities.h> so that ASPECT knows what a coordinate system is. Talk to me tomorrow if you want to know why it is not a problem on your laptop, but it is for the tester.
KerrMadeleine
force-pushed
the
grav_model_coords
branch
from
fde7be8 to
5588b37
Compare
|
/rebuild |
gassmoeller
approved these changes
Jun 6, 2024
|
Many thanks @KerrMadeleine! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The function expression plug-in for the gravity model can now evaluate in multiple coordinate systems. The plug-in was amended to take in the coordinate system variable by reading the "Coordinate system" parameter in the Function subsection and Gravity Model section of the parameter file. There are two added tests for the Gravity model function plug-in to ensure the cartesian and spherical functions evaluate as expected in a 2D cylindrical annulus.