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

[WIP] use SUNDIALS to compute reaction #5331

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jdannberg
Copy link
Contributor

First version of using SUNDIALS in the operator splitting routine. If you have any feedback on the general structure, let me know!
For now, I have ignored the possibility that temperature and composition might use different finite elements. I have also simply replaced the old version instead of checking if deal.ii is compiled with SUNDIALS.

I am also looking for feedback on how to pick the input parameters for ARKode.

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.

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.

Very cool. I think you're real close.

Comment on lines 1628 to 1620
pcout << " Solving composition reactions... " << std::flush;
pcout << " Solving composition reactions with SUNDIALS... " << std::flush;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just drop this change -- this will minimize the difference in the output files.

@@ -1694,72 +1686,82 @@ namespace aspect
// So even though we touch some DoF more than once, we always start from the same value, compute the
// same value, and then overwrite the same value in distributed_vector.
// TODO: make this more efficient.

using VectorType = Vector<double>;
SUNDIALS::ARKode<VectorType>::AdditionalData data (0.0, time_step, 0.001*time_step, 0.1*time_step, 1.e-6*time_step);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is hard to read. It corresponds to this function:

      AdditionalData(
        // Initial parameters
        const double initial_time      = 0.0,
        const double final_time        = 1.0,
        const double initial_step_size = 1e-2,
        const double output_period     = 1e-1,
        // Running parameters
        const double       minimum_step_size                     = 1e-6,
        const unsigned int maximum_order                         = 5,
        const unsigned int maximum_non_linear_iterations         = 10,
        const bool         implicit_function_is_linear           = false,
        const bool         implicit_function_is_time_independent = false,
        const bool         mass_is_time_independent              = false,
        const int          anderson_acceleration_subspace        = 3,
        // Error parameters
        const double absolute_tolerance = 1e-6,
        const double relative_tolerance = 1e-5);

Perhaps you can write this as

Suggested change
SUNDIALS::ARKode<VectorType>::AdditionalData data (0.0, time_step, 0.001*time_step, 0.1*time_step, 1.e-6*time_step);
SUNDIALS::ARKode<VectorType>::AdditionalData data (/* initial_time = */0.0,
/* end_time = */ time_step,
etc etc etc);

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separately, though, it would avoid potential bugs if you replaced the initial time zero with the actual current time, and the end time as current time + time step.

I would probably choose a larger initial time step than 0.001time_step. Do you actually need any output every 0.1time step? This introduces a time point that ARKode needs to hit, and if you don't need the actual output, just set that parameter to zero or equal to time_step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the formatting: I had to place the comment at the end for the indentation to look reasonable.

heating_model_manager.evaluate(in_C, out_C, heating_model_outputs_C);

for (unsigned int j=0; j<dof_handler.get_fe().base_element(introspection.base_elements.compositional_fields).dofs_per_cell; ++j)
std::vector<std::vector<double>> initial_values_C (quadrature_C.size(),std::vector<double> (introspection.n_compositional_fields));
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the declaration of this variable out of the loop over all cells. This way it needs to allocate memory only once.

// We have to store all values of the temperature and composition fields in one long vector
const unsigned int n_q_points = dof_handler.get_fe().base_element(introspection.base_elements.compositional_fields).dofs_per_cell;
const unsigned int n_fields = 1 + introspection.n_compositional_fields;
VectorType fields (n_q_points * n_fields);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this variable.

Comment on lines 1713 to 1718
for (unsigned int j=0; j<n_q_points; ++j)
for (unsigned int f=0; f<n_fields; ++f)
if (f==0)
fields[j*n_fields+f] = in_C.temperature[j];
else
fields[j*n_fields+f] = in_C.composition[j][f-1];
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to create a lambda function of the form

  auto copy_fields_into_one_vector = [n_q_points,n_fields](const auto &in, auto &fields) {...};

that you place somewhere up above, and then you here just have to do

  copy_fields_into_one_vector (in_C, fields);

Comment on lines 1725 to 1731
for (unsigned int j=0; j<n_q_points; ++j)
for (unsigned int f=0; f<n_fields; ++f)
{
for (unsigned int c=0; c<introspection.n_compositional_fields; ++c)
{
// simple forward euler
in_C.composition[j][c] = in_C.composition[j][c]
+ reaction_time_step_size * reaction_rate_outputs_C->reaction_rates[j][c];
accumulated_reactions_C[j][c] += reaction_time_step_size * reaction_rate_outputs_C->reaction_rates[j][c];
}
in_C.temperature[j] = in_C.temperature[j]
+ reaction_time_step_size * heating_model_outputs_C.rates_of_temperature_change[j];

if (temperature_and_composition_use_same_fe)
accumulated_reactions_T[j] += reaction_time_step_size * heating_model_outputs_C.rates_of_temperature_change[j];
if (f==0)
in_C.temperature[j] = y[j*n_fields+f];
else
in_C.composition[j][f-1] = y[j*n_fields+f];
Copy link
Contributor

Choose a reason for hiding this comment

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

This, too, could then be places into a local lambda function where then you just have to call copy_one_vector_into_fields(fields, in_C) or some such.

Names up for discussion of course.

Comment on lines -1817 to -1819
pcout << "in "
<< number_of_reaction_steps
<< " substep(s)."
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall if there was a way to ask the ARKode object for the number of steps it took. But if there isn't, then this will work:

unsigned int n_ode_timesteps = 0;
ode.output_step ([&n_ode_timesteps](const double       t,
                       const VectorType & sol,
                       const unsigned int step_number) {
  n_ode_timesteps = step_number;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, and the solve_ode function returns the final number of computed steps. So I could use that, but it might be a different number for each cell. So the question is: What would be a reasonable number to return? The average? The maximum? The sum?

@jdannberg
Copy link
Contributor Author

I addressed your comments (other than the last one about the steps), so my next question would be: how do we want to move forward with this? At the moment, we don't even have a section for the operator splitting in the timer output, so I do not know how this change affects the runtime. So that could be something to add in a separate PR?

We have some test cases with an analytical solution, so we do know that the new solution is more accurate now. But do we need to quantify this? Do we want to make the tolerance an input parameter?

Other than that, if this looks reasonable, I could now also add the implementation for the case where temperature and composition have different finite elements.

And finally, do we want to wait with this until we require SUNDIALS, or should I keep the old code and add a check if we have SUNDIALS?

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

2 participants