Taal: Hyperbolic diffusion and AMR controller #227

ranocha · 2020-10-14T13:52:09Z

I've added a new SteadyStateCallback (with optional absolute and relative tolerances). Moreover, I've renamed the IndicatorThreeLevel to ControllerThreeLevel as suggested by @gregorgassner in #219.

codecov · 2020-10-14T15:19:52Z

Codecov Report

Merging #227 into dev will increase coverage by 0.06%.
The diff coverage is 77.84%.

@@            Coverage Diff             @@
##              dev     #227      +/-   ##
==========================================
+ Coverage   89.52%   89.59%   +0.06%     
==========================================
  Files          58       60       +2     
  Lines       10438    10485      +47     
==========================================
+ Hits         9345     9394      +49     
+ Misses       1093     1091       -2

Impacted Files	Coverage Δ
src/Trixi.jl	`100.00% <ø> (ø)`
src/callbacks/callbacks.jl	`80.00% <ø> (ø)`
src/solvers/dg/dg_1d.jl	`86.70% <ø> (ø)`
src/solvers/dg/dg_2d.jl	`90.97% <ø> (ø)`
src/equations/equations.jl	`83.33% <66.66%> (ø)`
src/callbacks/steady_state.jl	`68.42% <68.42%> (ø)`
src/equations/2d/hyperbolic_diffusion.jl	`86.77% <70.31%> (+3.24%)`	⬆️
src/callbacks/analysis.jl	`83.41% <75.00%> (+3.11%)`	⬆️
src/callbacks/amr_dg1d.jl	`94.78% <80.00%> (-0.09%)`	⬇️
src/callbacks/amr_dg2d.jl	`95.03% <80.00%> (-0.07%)`	⬇️
... and 8 more

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 3721aa6...2ede36a. Read the comment docs.

…ssner

…ssner

andrewwinters5000 · 2020-10-14T18:17:11Z

examples/2d/elixir_hyp_diff_harmonic_nonperiodic.jl

+resid_tol = 5.0e-12 # TODO: Taal, move this parameter to the callback
+equations = HyperbolicDiffusionEquations2D(resid_tol)
+
+initial_conditions = Trixi.initial_conditions_harmonic_nonperiodic


I noticed that sometimes we use, e.g., initial_conditions = Trixi.intial_conditions_harmonic_nonperiodic whereas in my elixir conversions I simply used, e.g., initial_conditions = initial_conditions_rotor. Is this a matter of convention or should I edit my elixir files to do it like this?

What you did is perfectly fine. We need to decide which boundary conditions to keep in Trixi, see #207. If you have some time, it would be nice if you can note your thoughts on this there.

That's marked as a blocking issue before we merge Taal (#200) into master.

andrewwinters5000 · 2020-10-14T18:20:05Z

examples/2d/elixir_hyp_diff_nonperiodic.jl

+equations = HyperbolicDiffusionEquations2D(resid_tol)
+
+initial_conditions = Trixi.initial_conditions_poisson_nonperiodic
+boundary_conditions = (Trixi.boundary_conditions_poisson_nonperiodic,


I forget, is there a schematic anywhere of which entry of the four component array corresponds to "bottom" "top" "left" or "right"? This could be useful documentation for new users, or me for that matter :)

Good point! We use the usual numbering of the directions, i.e. 1 => -x, 2 => +x, 3 => -y, 4 => +y. I've added a comment here and started to documents some of these conventions in d8f7b06

andrewwinters5000

Overall very nice! I left two comments for clarification in the new hyperbolic diffusion elixir files, but I think this is good to go.

For the multiphysics runs, should the euler + gravity also use the SteadyStateCallback or does it detect steady state another way?

ranocha · 2020-10-15T04:47:46Z

For the multiphysics runs, should the euler + gravity also use the SteadyStateCallback or does it detect steady state another way?

We don't need the SteadyStateCallback there because we use our home-grown time integration methods for the hyperbolic diffusion part there. The relevant code is in src/semidiscretization_euler_gravity.jl, starting in line 190.

ranocha · 2020-10-15T05:20:23Z

Memo: Merge #223 into dev, fix conflicts here, then merge this PR into dev.

andrewwinters5000 · 2020-10-15T05:21:19Z

We don't need the SteadyStateCallback there because we use our home-grown time integration methods for the hyperbolic diffusion part there. The relevant code is in src/semidiscretization_euler_gravity.jl, starting in line 190.

Ah gotcha. I forgot that the callback structure is for the ODE package and not the in-house integrators

sloede · 2020-10-20T06:18:37Z

src/callbacks/alive.jl

-  condition = (u, t, integrator) -> alive_interval > 0 && ((integrator.iter % alive_interval == 0 && (analysis_interval == 0 || integrator.iter % analysis_interval != 0)) || t == integrator.sol.prob.tspan[2])
+  condition = (u, t, integrator) -> alive_interval > 0 && (
+    (integrator.iter % alive_interval == 0 && (analysis_interval == 0 || integrator.iter % analysis_interval != 0)) ||
+    t == integrator.sol.prob.tspan[2] || isempty(integrator.opts.tstops))


What does isempty(integrator.opts.tstops) check for? Is this non-empty if another callback marked this as the last time step and that the solver finishes afterwards?

sloede · 2020-10-20T06:21:20Z

src/callbacks/amr.jl

-    IndicatorThreeLevel(semi, indicator; base_level=1,
-                                         med_level=base_level, med_threshold=0.0,
-                                         max_level=base_level, max_threshold=1.0)
+    ControllerThreeLevel(semi, indicator; base_level=1,


TBH, I do not like the name Controller here, as it gives no indication of what is being controlled. I have a similar beef with Indicator, but since that's used for multiple purposes, I was not able to come up with a better name. However, here I'd suggest to call the AMR Controllers AmrController (or AMRController). What do you think @ranocha?

sloede · 2020-10-20T06:23:30Z

src/callbacks/analysis.jl

@@ -337,6 +339,7 @@ function (cb::DiscreteCallback{Condition,Affect!})(sol::ODESolution) where {Cond
  @unpack analyzer = analysis_callback

  l2_error, linf_error = calc_error_norms(sol.u[end], sol.t[end], analyzer, semi)
+  (; l2=l2_error, linf=linf_error)


Shouldn't this be

return (l2=l2_error, linf=linf_error)

?

sloede · 2020-10-20T06:28:42Z

src/callbacks/steady_state.jl

+  reltol::RealT
+end
+
+function SteadyStateCallback(; abstol=1.0e-8, reltol=1.0e-6)


Are we sure that default tolerances make sense here? I am really asking, maybe they do, but intuitively I would say that this is so application dependent it does not make much sense (and that instead we should force the users to pick values.

sloede · 2020-10-20T09:55:08Z

src/callbacks/amr.jl

 #       But that would remove the simplest possibility to write that stuff to a file...
 #       We could of course implement some additional logic and workarounds, but is it worth the effort?
-function (indicator::IndicatorThreeLevel)(u::AbstractArray{<:Any},
-                                          mesh::TreeMesh, equations, dg::DG, cache)
+function (controller::ControllerThreeLevel)(u::AbstractArray{<:Any},


I wonder if it is really necessary here to make this a callable instead of a free function (I've left a similar comment elsewhere). We don't have to discuss it here (I'll put it on the next meeting agenda), but maybe for this specific case there's a strong rationale?

ranocha requested review from sloede, gregorgassner and andrewwinters5000 October 14, 2020 13:52

ranocha added 9 commits October 14, 2020 17:59

rename u to u_ode for consistency

a2b8fbc

boundscheck only as discussed in trixi-framework#210

432403b

WIP: hyperbolic diffusion example

5eb2472

SteadyStateCallback for hyperbolic diffusion

50d8279

finished 2D hyp. diffusion in Taal

936da6f

IndicatorThreeLevel -> ControllerThreeLevel as suggested by @gregorga…

ea1ea1d

…ssner

trixi_include can also replace keyword assignments

1bdccc6

fix missed renaming of indicator->controller

5e22ec6

adapt new elixirs to ControllerThreeLevel

4a79919

ranocha force-pushed the hr/taal branch from 0afef1a to 4a79919 Compare October 14, 2020 16:05

andrewwinters5000 reviewed Oct 14, 2020

View reviewed changes

andrewwinters5000 approved these changes Oct 14, 2020

View reviewed changes

comment on directions for BCs

d8f7b06

ranocha added 2 commits October 15, 2020 08:08

update codecov settings

3caa786

Merge branch 'dev' into hr/taal

2ede36a

ranocha merged commit 58ac9bd into trixi-framework:dev Oct 15, 2020

sloede mentioned this pull request Oct 20, 2020

Taal: Should we use a named tuple for boundary conditions? #238

Closed

sloede reviewed Oct 20, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taal: Hyperbolic diffusion and AMR controller #227

Taal: Hyperbolic diffusion and AMR controller #227

ranocha commented Oct 14, 2020

codecov bot commented Oct 14, 2020 •

edited

Loading

andrewwinters5000 Oct 14, 2020

ranocha Oct 15, 2020

ranocha Oct 15, 2020

andrewwinters5000 Oct 14, 2020

ranocha Oct 15, 2020

andrewwinters5000 left a comment

ranocha commented Oct 15, 2020

ranocha commented Oct 15, 2020

andrewwinters5000 commented Oct 15, 2020

sloede Oct 20, 2020

sloede Oct 20, 2020

sloede Oct 20, 2020

sloede Oct 20, 2020

sloede Oct 20, 2020

Taal: Hyperbolic diffusion and AMR controller #227

Taal: Hyperbolic diffusion and AMR controller #227

Conversation

ranocha commented Oct 14, 2020

codecov bot commented Oct 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewwinters5000 left a comment

Choose a reason for hiding this comment

ranocha commented Oct 15, 2020

ranocha commented Oct 15, 2020

andrewwinters5000 commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2020 •

edited

Loading