-
Notifications
You must be signed in to change notification settings - Fork 251
Adding tidal potential to gravity model #6614
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
base: main
Are you sure you want to change the base?
Conversation
Corrections are done but I am not sure why I have failure from dealii 9.5 test. |
@hyunseong96 Thank you for this patch! I will look at it later this week, but in the meantime, can you take a look at the test failure? It currently reports this:
|
@bangerth Thanks in advance! :) The issue with test is solved now. |
Hi @bangerth , hope you are doing well! just a gentle reminder to ask for reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will go ahead and leave some comments, Wolfgang can of course chime in on the mathematics (I havent compared the implemented formula with the paper). Some comments on the code and structure below. I think this is a valuable addition, but I would change the structure of the code a bit. Let me know in case you have questions or want some guidance on how to do something.
|
||
|
||
|
||
template <int dim> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I explain in #6681 we usually associate one class/plugin with one file. The radial gravity models were an exception from the earliest days of ASPECT's life. Can you introduce your gravity profile in a new file instead? For reasons I will explain below I would name the new file (and class and everything) RadialWithTidalPotential
and radial_with_tidal_potential.h/cc
respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make it into a different plugin file then!
|
||
|
||
template <int dim> | ||
class RadialLinearWithTidalPotential : public RadialLinear<dim> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it works to derive a plugin from another plugin if you want to use the behavior of the parent class in the child class, I have noted over the years that it is rarely a good idea in ASPECT. It creates complicated class structures, and sometimes it straight out prevents making full use of the parent class (in particular if the parent has protected
member variables or is itself derived from another class like here). I would recommend the following approach instead:
class RadialLinearWithTidalPotential : public RadialLinear<dim> | |
class RadialLinearWithTidalPotential : public Interface<dim>, public SimulatorAccess<dim> |
Then give this class a private member variable of type RadialLinear<dim>
as a "base_gravity_model" and use its functionality by calling functions of this member variable. You can see how this works in the class initial_temperature/patch_on_S40RTS.h
and its member variable S40RTSPerturbation<dim> s40rts;
. Note in particular the lines 166-168 in initial_temperature/patch_on_S40RTS.cc
where your class would initialize the member variable and let it read its own input parameters and line 76 where it uses its function get_Vs
.
For now this is just a way to achieve the same functionality as your current approach (just with a bit more code, but easier to understand concept). But in the future this allows to expand the plugin to work with more than just the radial linear gravity model. For example the material model prescribed_viscosity
can use an arbitrary material model as base model, and then modify it. In the same way your plugin could use any of the radial gravity profiles and apply a tidal potential on top.
|
||
|
||
|
||
template <int dim> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add doxygen documentation above the class name about what this class does. You can use the text you wrote at the bottom of the source file, but bring it into the doxygen format (/** ... */). This will be used in the online documentation (here the example for RadialLinear
: https://aspect.geodynamics.org/doc/doxygen/classaspect_1_1GravityModel_1_1RadialLinear.html.
double b_NSR; | ||
|
||
/** | ||
* Radius of modelling satellite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By satellite you mean the moon/planet this gravity is computed for? This would be worth spelling out, since it may not be obvious for geoscientists focused on Earth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I haven't thought about the planet's perspective. My idea was about effect of gravitation by planet on satellite, because I wanted to implement gravity variation on Europa. That's why I used host planet, which affects the gravity of satellite.
Would it be better to differentiate as modeling object and gravitationally interacting object?
I also think I should add few more comments on parameters for cases other than icy moons.
double a_s; | ||
|
||
/** | ||
* Rate of non-synchronous rotation in m/year |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use SI units wherever possible. You can still allow m/yr as input parameter if the global parameter Use years instead of seconds
is set, and then convert into m/s when reading and storing the parameter.
And why m as length unit? Wouldnt it be much easier to ask the user to compute the angular velocity themselves beforehand and use that as input? Then you dont need to input the radius of the satellite either, or do you need that for anything else?
source/gravity_model/radial.cc
Outdated
{ | ||
M_p = prm.get_double ("Mass of planet"); | ||
a_s = prm.get_double ("Semimajor axis of satellite"); | ||
b_NSR = prm.get_double ("Rate of nonsynchronous rotation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you can do the following to allow for different time units in the input file, but still use SI units in the code:
b_NSR = prm.get_double ("Rate of nonsynchronous rotation"); | |
const double time_scale = this->get_parameters().convert_to_years ? constants::year_in_seconds : 1.0; | |
b_NSR = prm.get_double ("Rate of nonsynchronous rotation") / time_scale; |
source/gravity_model/radial.cc
Outdated
{ | ||
prm.declare_entry ("Mass of planet", "1.898e27", | ||
Patterns::Double (), | ||
"Mass of satellte's host planet. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Mass of satellte's host planet. " | |
"Mass of satellite's host planet. " |
source/gravity_model/radial.cc
Outdated
"Units is $kg$."); | ||
prm.declare_entry ("Semimajor axis of satellite", "670900000", | ||
Patterns::Double (), | ||
"Length of semimajor axis of satellite. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to be more verbose:
"Length of semimajor axis of satellite. " | |
"Length of the semimajor axis of the orbit of the modeled planet. " |
source/gravity_model/radial.cc
Outdated
void | ||
RadialLinearWithTidalPotential<dim>::parse_parameters (ParameterHandler &prm) | ||
{ | ||
AssertThrow (dim==3, ExcMessage ("The 'radial linear with tidal potential' gravity model " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update name
source/gravity_model/radial.cc
Outdated
prm.enter_subsection("Radial linear with tidal potnetial"); | ||
{ | ||
M_p = prm.get_double ("Mass of planet"); | ||
a_s = prm.get_double ("Semimajor axis of satellite"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current spelling is easy to confuse, if you model an elliptical body on an elliptical orbit.
a_s = prm.get_double ("Semimajor axis of satellite"); | |
a_s = prm.get_double ("Semimajor axis of orbit"); |
@gassmoeller Hi Rene, thanks for reviewing the PR. I have few questions.
Did you mean the parameter, b_NSR, as angle/year? or is there any perspective I am not aware of inside the code while solving equations? Second, separating into different files seems a bit risky with my poor skill of git commands sadly. I found out that I need to add this line to radial constant plugin. After the incident of having hundred of others' commits on my PR at Hackathon, I am still not sure how I can update the branch so I can adjust my plugin to the update you will do about radial plugins. I searched for this and tried on branch I am not using anymore.
Would this four steps be valid to keep up with updates? Otherwise I think it can be easier for me to close the PR and open a new one after your radial separation PR is merged. |
Yes I could have been more clear in my comment there. In general there are two separate issues:
I do not see why you would need to do this. You can declare a class (in this
Please do not use this workflow, it is what caused all the commits to appear in your branch. It is a valid git workflow and some projects use it, but we do not. I could explain our alternate workflow and the reasons for it (using an operation called |
@gassmoeller Thanks for clarifying! I changed parameters accordingly. I will modify the tests later once codes are sorted out!
I can push the ones with public variables with errors if you want it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, much better structure. I left a number of comments, but they are mostly suggestions for improvements, I didnt see anything wrong.
Based on the error messages that you posted and your questions, I think it is worth for you to review the difference between static functions and non static functions of a class in c++. Here is a short summary: https://www.learncpp.com/cpp-tutorial/static-member-functions/.
In short, 'normal' member functions of a class (say class RadialConstant
) require an object of this class to be created (in your case radialconstant
). Then you can call the member functions like so: radialconstant.gravity_vector(...)
. Static member functions however do not require an object of that class, and are usually called through their class name (instead of a particular object name), like so: RadialConstant<dim>::declare_parameters(...)
.
return Tensor<1,dim>(); | ||
|
||
const double r = p.norm(); | ||
return -radialconstant.magnitude * p/r + tidal_gravity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, here is where you access the private member variable magnitude
of the object radialconstant
which is of the class type RadialConstant<dim>
. The way to work around that is to call the public member function gravity_vector
like so:
return radialconstant.gravity_vector(p) + tidal_gravity;
Then you do no longer access the private member variable magnitude, and instead call the public member function gravity_vector
.
@@ -0,0 +1,4 @@ | |||
Added: ASPECT now has a new gravity model plugin called 'Radial linear with tidal potnetial' for tidal forces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added: ASPECT now has a new gravity model plugin called 'Radial linear with tidal potnetial' for tidal forces. | |
Added: ASPECT now has a new gravity model plugin called 'Radial with tidal potential' for tidal forces. |
|
||
template <int dim2> friend class RadialWithTidalPotential; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this
{ | ||
/** | ||
* A class that describes gravity as a radial vector of linearly | ||
* changing magnitude by tidal potential from flattening and non-synchronnous rotation with respect to position and time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* changing magnitude by tidal potential from flattening and non-synchronnous rotation with respect to position and time. | |
* changing magnitude, which is modified by a tidal potential from flattening and non-synchronous rotation. |
* The equation implemented in this gravity model is from Tobie et al. (2025) (https://doi.org/10.1007/s11214-025-01136-y), | ||
* which is defined as: | ||
* g = -magnitude - gradient (-(tidal potential)). | ||
* Tidal potential is positive because it is from geodesy research, where potential is taken as positive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Tidal potential is positive because it is from geodesy research, where potential is taken as positive. | |
* Tidal potential is positive because the formula follows conventions from geodesy research, where potential is taken as positive. |
void | ||
RadialWithTidalPotential<dim>::parse_parameters (ParameterHandler &prm) | ||
{ | ||
AssertThrow (dim==3, ExcMessage ("The 'radial with tidal potential' gravity model " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so you only allow this for 3D models. Please add this to the documentation at the bottom of this file. I will leave a comment above for how to handle this in the gravity_vector
function.
template <int dim> | ||
Tensor<1,dim> | ||
RadialWithTidalPotential<dim>::gravity_vector (const Point<dim> &p) const | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you want this to only work in 3D models, but ASPECT is compiled for 2D and 3D models, you will (or maybe already have) run into trouble with using types like Tensor<1,dim>
. When ASPECT is compiled for dim==2
the compiler will notice you try to access elements that dont exist in 2D and likely complain. Do the following to work around this (this is taken from here):
template <int dim> | |
Tensor<1,dim> | |
RadialWithTidalPotential<dim>::gravity_vector (const Point<dim> &p) const | |
{ | |
template <int dim> | |
Tensor<1,dim> | |
RadialWithTidalPotential<dim>::gravity_vector (const Point<dim> &p) const | |
{ | |
// This plugin is not implemented for 2D models | |
AssertThrow(false, ExcNotImplemented()); | |
return Tensor<1,dim>(); | |
} | |
template <> | |
Tensor<1,3> | |
RadialWithTidalPotential<dim>::gravity_vector (const Point<3> &p) const | |
{ | |
const unsigned int dim = 3; |
This construct is called a template specialization (more information here: https://www.learncpp.com/cpp-tutorial/function-template-specialization/), and it provides different functions for your class for dim==2 (which will crash) and dim==3 (which will work as you expect it to).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! but I had to change a bit to make this code work. I hope the code below is the correct modification.
template <>
Tensor<1,3>
RadialWithTidalPotential<3>::gravity_vector (const Point<3> &p) const
{
I will use this later for Tensor<1,2> when Antoniette and I decide how to implement 2D case.
So far, we had discussion how to implement this plugin in 2D but we will make it as different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gassmoeller , I have a bit of trouble for upper part of suggestion, which is template
Tensor<1,dim> RadialWithTidalPotential<dim>::gravity_vector (const Point<dim> &p) const { // This plugin is not implemented for 2D models AssertThrow(false, ExcNotImplemented()); return Tensor<1,dim>(); }
While in make, I see the warning that warning:
unused parameter 'p' [-Wunused-parameter]
Because tests considers warnings as errors, I fail tests. Is there a simple but reasonable way I can use Point 'p'? or the way to avoid this warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, our usual way to avoid this warning is to comment the name (but not the type) of the parameter argument. This tells the compiler that this parameter exists (for some reasons or other), but that we are intentionally not using it in the function:
RadialWithTidalPotential<dim>::gravity_vector (const Point<dim> &/*p*/) const
{ | ||
M_p = prm.get_double ("Mass of perturbing body"); | ||
a_s = prm.get_double ("Semimajor axis of orbit"); | ||
const double time_scale = this->get_parameters().convert_to_years ? constants::year_in_seconds : 1.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so you do the conversion already. Then disregard my earlier comment, and just fix the documentation of this parameter to say that it is given in degrees/year or degrees/second depending on the input parameter 'Use years instead of seconds'.
M_p = prm.get_double ("Mass of perturbing body"); | ||
a_s = prm.get_double ("Semimajor axis of orbit"); | ||
const double time_scale = this->get_parameters().convert_to_years ? constants::year_in_seconds : 1.0; | ||
b_NSR = prm.get_double ("Angular rate of nonsynchronous rotation") * constants::degree_to_radians / time_scale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but according to your formulas, shouldnt this parameter now be b
and not b_NSR
?
} | ||
else | ||
{ | ||
Assert (false, ExcMessage ("This initial condition can only be used if the geometry " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert (false, ExcMessage ("This initial condition can only be used if the geometry " | |
Assert (false, ExcMessage ("This gravity model can only be used if the geometry " |
Apologies for opening this PR so late. Antoniette and I took some time to check sign convention for the equation.
I open this PR to add gravity model plugin where I can add tidal potential exerted as satellite orbits around the host planet.
The equation of tidal potential is from Tobie et al. (2025) (https://doi.org/10.1007/s11214-025-01136-y).

I only use T* and T0 because ep 4, 5 and 6 are dependent to short time scale (~3.55 days) and they are replaced by tidal heating.
T* is flattening and T0 is nonsynchronous rotation. T0 happens because ice surface is decoupled from rocky interior by the subsurface ocean and have difference rotational velocity.
To implement in cartesian coordinates, these equations are converted to ones using x, y and z instead of spherical variables.
which ends up as:
dV_T/dx = (3* G M_p)/(2 a^3) (1/6(2x)+1/2(2C1x - 2C2y))
dV_T/dy = (3* G M_p)/(2 a^3) (1/6(2y)+1/2(2C1y - 2C2x))
dV_T/dz = (3* G M_p)/(2 a^3) (1/6(-4z)).
where G is gravitational constant, M_p is mass of perturbing body, a is semi-major axis, C1 = cos(2bt) and C2 = sin(2b*t).
This$V_T$ is from geodesy research, which takes potential and acceleration always positive as unit vector is inward to center of body. Therefore, for us, where unit vector is outward from the center of body, the return of plugin is $-g\vec{r}-\nabla(-V_T)\vec{r}$
test model is just to see how gravity changes with time. temperature does not change with time. viscosity is high throughout the model so there is no convection. Colorbar is for gravity magnitude, and arrow denotes direction of tidal acceleration.

I will edit some parts quickly in test .prm, make more clarification in .cc and add change file!