ResetTimeStepEstimate logic

[dylan-copeland]

I think it would be better to have ResetTimeStepEstimate() also set qdata_is_current to false. Currently, it only sets dt_est to infinity. Since ResetTimeStepEstimate() is called between ode_solver steps, qdata_is_current is already set to false by LagrangianHydroOperator::Mult, so there is no bug. However, if someone introduces code that calls UpdateQuadratureData, which is a public function, then it will set qdata_is_current to true, and the infinite dt_est will be used rather than recomputing it.

[vladotomov]

Sorry @dylan-copeland, somehow I've missed this.

I don't quite follow the use case. Calling UpdateQuadratureData always recomputes dt_est, so it won't be infinite after the call. Also, the quadrature data should be independent of the dt_est.

[dylan-copeland]

@vladotomov The usual usage, which is fine, is the following:

for each timestep: hydro.ResetTimeStepEstimate(); // sets qdata.dt_est = infinity ode_solver->Step: call LagrangianHydroOperator::Mult UpdateQuadratureData solve for velocity, energy qdata_is_current = false;

The potential problem is if someone does something like this:

for each timestep: UpdateQuadratureData // sets qdata_is_current = true hydro.ResetTimeStepEstimate(); // sets qdata.dt_est = infinity ode_solver->Step: call LagrangianHydroOperator::Mult UpdateQuadratureData // does nothing (returns since qdata_is_current == true), leaving dt_est = infinity solve for velocity, energy qdata_is_current = false;

Of course, this should not be done, but it is not clear from the interface, and the code allows it, resulting in infinite dt_est. If ResetTimeStepEstimate sets qdata_is_current to false, then UpdateQuadratureData will compute dt_est.

My original post stated that UpdateQuadratureData is public, but it is actually protected. That should help prevent this. I must have discovered this through some convoluted reduced order modeling code, so it probably will not be cause trouble for most people. So if you do not agree that qdata_is_current would make the code clearer, we can just close this issue.

[vladotomov]

Yes, doing

1. GetTimeStepEstimate (this makes a call to UpdateQuadratureData)
2. ResetTimeStepEstimate
3. Step

would break the method, but it also makes no sense to call a time integrator with undefined time step.

I don't think we can prevent all use cases, and that's not the purpose of the miniapp. Also note that implementing your suggestion would change the code path (there will be extra UpdateQuadratureData computations, as the Reset is right before Step in the current code). So I'm inclined not to change it. Do you agree?

[dylan-copeland]

Yes, I agree not to change the code. At first it seemed like a simple fix to avoid potential bugs, but it is not so simple, and we don't necessarily have to prevent all misuses of the code. Maybe an assertion somewhere that dt_est is not infinity would be a minimal way to catch bugs like this.

[vladotomov]

Yes that would be good.