dougshidong / PHiLiP

Parallel High-Order Library for PDEs through hp-adaptive Discontinuous Galerkin methods
Other
45 stars 37 forks source link

Time Refinement Study using flow_solver #138

Closed cpethrick closed 2 years ago

cpethrick commented 2 years ago

Closes issue #115. Changes are as follows:

dougshidong commented 2 years ago

Wondered if there was a need for physics/exact_solutions as per previous discussion on Alex's PR about adding a time component to either manufactured solutions or initial conditions. Also, manufactured solutions and initial conditions are already very similar in that they basically just override the dealii::Function which already has a way to modify a time component

https://www.dealii.org/developer/doxygen/deal.II/classFunction.html https://www.dealii.org/developer/doxygen/deal.II/classFunctionTime.html#a3a583fd8f30db3549dbaa43b2592e2bc

At this point, I'm wondering if we even need to have different classes such as ManufacturedSolution, InitialConditions, and ExactSolution, and if we should instead just have a single SolutionFunctionFactory for all three (especially since some initial conditions are just the exact solutions at time t = 0).

cpethrick commented 2 years ago

The most recent commit fixes any minor comments. I've marked conversations that are quick fixes as resolved.

I think more discussion about the initial condition / exact solution / manufactured solution redundancy is needed. I'll summarize the discussion from the PR comments:

I suppose that the unified "analytical function" would not be in the scope of this PR. Therefore, should I remove exact_solution and group the solution into initial_condition, despite the naming? Or leave exact_solution as I've implemented it as temporary functionality?

dougshidong commented 2 years ago

The most recent commit fixes any minor comments. I've marked conversations that are quick fixes as resolved.

I think more discussion about the initial condition / exact solution / manufactured solution redundancy is needed. I'll summarize the discussion from the PR comments:

  • Could make a unified "analytical function" to replace the three aforementioned analytical functions.
  • The exact_solution function as I've implemented it should take time as an argument rather than as a constant. This would be necessary (or at least useful) for comparing solutions that reach a different final_time than the target.
  • As suggested by Doug, I will look into dealii::Function get_time, set_time, advance_time to see if they make exact_solution better.

I suppose that the unified "analytical function" would not be in the scope of this PR. Therefore, should I remove exact_solution and group the solution into initial_condition, despite the naming? Or leave exact_solution as I've implemented it as temporary functionality?

I'm fine with not unifying the three aforementionned classes. It does become out of scope. Though, please do open an issue an link up that comment: https://github.com/dougshidong/PHiLiP/pull/138#issuecomment-1118967390

Pretty sure that using deal.II get_time, set_time, advance_time will solve that time input parameter stuff. You basically just have to call exact_solution.set_time(t) before calling exact_solution.value(t), and use const double t = get_time(), inside of value().

sivanadarajah commented 2 years ago

Wondered if there was a need for physics/exact_solutions as per previous discussion on Alex's PR about adding a time component to either manufactured solutions or initial conditions. Also, manufactured solutions and initial conditions are already very similar in that they basically just override the dealii::Function which already has a way to modify a time component

https://www.dealii.org/developer/doxygen/deal.II/classFunction.html https://www.dealii.org/developer/doxygen/deal.II/classFunctionTime.html#a3a583fd8f30db3549dbaa43b2592e2bc

At this point, I'm wondering if we even need to have different classes such as ManufacturedSolution, InitialConditions, and ExactSolution, and if we should instead just have a single SolutionFunctionFactory for all three (especially since some initial conditions are just the exact solutions at time t = 0).

You would still want to be able to have a different manufactured solutions then the prescribed initial condition. Wouldn't you loose that capability if you have a single SolutionFunctionFactory?

dougshidong commented 2 years ago

Wondered if there was a need for physics/exact_solutions as per previous discussion on Alex's PR about adding a time component to either manufactured solutions or initial conditions. Also, manufactured solutions and initial conditions are already very similar in that they basically just override the dealii::Function which already has a way to modify a time component https://www.dealii.org/developer/doxygen/deal.II/classFunction.html https://www.dealii.org/developer/doxygen/deal.II/classFunctionTime.html#a3a583fd8f30db3549dbaa43b2592e2bc At this point, I'm wondering if we even need to have different classes such as ManufacturedSolution, InitialConditions, and ExactSolution, and if we should instead just have a single SolutionFunctionFactory for all three (especially since some initial conditions are just the exact solutions at time t = 0).

You would still want to be able to have a different manufactured solutions then the prescribed initial condition. Wouldn't you loose that capability if you have a single SolutionFunctionFactory?

Not really, that SolutionFunctionFactory would be responsible for returning a dealii::Function, which can then be used to either initialize our initial conditions or even to compare the final solution.

class ExponentialSolutionFunction : public dealii::Function
{
    real value(dealii::Point p) const override {
        // implementation
    }
    dealii::Tensor gradient(dealii::Point p) const override {
        // implementation
   }
}
class SineSolutionFunction : public dealii::Function
{
    real value(dealii::Point p) const override {
        // implementation
    }
    dealii::Tensor gradient(dealii::Point p) const override {
        // implementation
   }
}
class SolutionFunctionFactory
{
    std::shared_ptr<dealii::Function> createSolutionFunction(const SolutionFunctionType function_type)
    {
        switch(function_type) {
            case(SolutionFunctionType::ExponentialSolution) return std::make_shared<ExponentialSolutionFunction>(); break;
            case(SolutionFunctionType::SineSolution) return std::make_shared<SineSolutionFunction>(); break;
            // etc.
        }
    }
}

Then in the code, the resulting dealii::Function can be used in various ways:

void test()
{
    // some initialization
    // WAY #1
    std::shared_ptr<dealii::Function> initial_conditions_function = createSolutionFunction(function_type)
    VectorTools::project ( mapping, dof, constraints, quadrature, 
                                       *initial_conditions_function, // used here
                                       dg->solution);
    // WAY #2
     std::shared_ptr<dealii::Function> manufactured_solution_function = createSolutionFunction(other_function_type);
    // Somehow use manufactured_solution_function to define source terms within Physics/DG

    // Some other code to run the solver

    // WAY #3
    std::shared_ptr<dealii::Function> exact_solution = createSolutionFunction(another_function_type_again);
    // Code to compare final dg->solution to exact_solution

Being able to update the time parameter is already a feature of the dealii::Function. Right now, our ManufacturedSolution, InitialConditions, and ExactSolution, are a layer of abstraction that don't really add to the code in terms of functionality since the implementations are in another layer of derived classes.

We basically have: ............................/---- ManufacturedSolution --- Bunch of different ManufacturedSolutions dealii::Function --|---- InitialConditions --- Bunch of different InitialConditions ............................ \ ---- ExactSolution --- Bunch of different ExactSolution

But the ManufacturedSolution, InitialConditions, ExactSolution, have the exact same code.

jbrillon commented 2 years ago

Just to add when you say they have the "exact same code", the only difference is that the ExactSolutionFunction and InitialConditionFunction classes only require value to be defined by their derived classes, whereas ManufacturedSolution requires value, gradient, and hessian to be defined by the derived classes; so really we'd only be unifying the value member function aspect of the classes.

Unifying them would be good, at least unifying InitialConditionFunction and ExactSolutionFunction would be a good start

dougshidong commented 2 years ago

Just to add when you say they have the "exact same code", the only difference is that the ExactSolutionFunction and InitialConditionFunction classes only require value to be defined by their derived classes, whereas ManufacturedSolution requires value, gradient, and hessian to be defined by the derived classes; so really we'd only be unifying the value member function aspect of the classes.

Unifying them would be good, at least unifying InitialConditionFunction and ExactSolutionFunction would be a good start

Yeah, I think that ideally, we're able to have a generic layer of SolutionFunction that computes gradient and Hessian with automatic differentiation (or finite difference). That way, no one has to worry about implementing them anymore and we can simply define value. Even for ExactSolution, there might be cases where you want to know the exact solution gradient since the gradient values in DG also have an expected convergence order.

cpethrick commented 2 years ago

My recent commits address all comments made on this PR, as well as merging in the most recent master. I will open an issue regarding the unification of the various dealii::Function codes and include the highlights of the discussion here.

dougshidong commented 2 years ago

My recent commits address all comments made on this PR, as well as merging in the most recent master. I will open an issue regarding the unification of the various dealii::Function codes and include the highlights of the discussion here.

Thanks. Will resolve and approve when the issue is opened.

dougshidong commented 2 years ago

Looks like everything got resolved or an issue has been opened. @jbrillon and @donovan97 , please resolve the comments and merge it in.

dougshidong commented 2 years ago

Thanks again @cpethrick ! Bigger review than I expected, but it happens.