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

Avoid a warning about an unused variable. #808

Closed
wants to merge 1 commit into from

Conversation

bangerth
Copy link
Contributor

@bangerth bangerth commented Apr 7, 2016

This is in one of the temperature assembly functions, where we now apparently no longer
need to check whether we use the BDF2 formula or a backward Euler for the first
time step. It would actually be interesting to know why we no longer need to check this,
i.e., whether we accidentally lost some functionality in the transition, or whether the
function was just split into two and the variable is only needed in one.

@gassmoeller
Copy link
Member

This is already addressed in #783. It guess it was a copy-paste issue, because it is declared in local_assemble_advection_face_terms and there it is never used, but it is also declared in local_assemble_advection_system and there it is used. I guess @spco copied the latter function to create the former. Whether we would need to consider the time-stepping scheme used somewhere in local_assemble_advection_face_terms I do not know.

@gassmoeller
Copy link
Member

This would also create conflicts with #775, which is a much larger change. Maybe we can postpone this after that one is merged?

@bangerth
Copy link
Contributor Author

bangerth commented Apr 7, 2016

OK, let's give @spco the opportunity to chime in.

I don't mind if we don't merge it, although it's likely going to be really easy to rebase the other patches on top of this one if necessary. But it's not an important point. I'd like to keep it open to see what @spco has to say.

@spco
Copy link
Contributor

spco commented Apr 7, 2016

You're right - use_bdf2_scheme is not needed in face terms as they are multiplied by time_step in both cases. Please delete this copy-paste hangover!

@bangerth
Copy link
Contributor Author

bangerth commented Apr 7, 2016

I see. @spco -- is this because you always use an implicit Euler or Crank-Nicolson scheme for the boundary terms? Could things be made more accurate if one switched to a BDF-2 scheme? Or is this simply not an option for the face terms?

@spco
Copy link
Contributor

spco commented Apr 7, 2016

The only term that needs any timestepping is the time-derivative, which is a cell-only term. The weighting of the face terms by time_step is just to ensure they scale correctly with the rest. I don't know that using a weighting like in field_term_for_rhs would help this scaling - this would need to be tested numerically to see if that scaling had a better effect on condition number etc. I don't think it would be much gain either way, and could well be a loss.

@bangerth
Copy link
Contributor Author

bangerth commented Apr 7, 2016

I think what I meant is something like this: Let's say you had an equation of the form d/dt u + u = f. Then clearly you need to decide what to do with the time derivative, and there are many ways to do it. But you also have to decide what you want to do with u. You can consider it as fully implicit, and then it is part of the left hand side and you get an addition to the matrix. This seems to me what you are doing -- where the contribution to the matrix has a weighting factor that also depends on the time step size.

But you could also replace u by u^{n-1} to make things explicit, and then it becomes part of the right hand side. Or you could do something like the Crank-Nicolson, where you replace u by 1/2 (u^n + u^{n-1}) so that it goes both to the left and right hand sides. In the case of the BDF-2 method, u^{n-2} would in fact also make an appearance somewhere, but this clearly only works if you are at least in the time step 2, and so what we do is ask whether we're in the first time step or in following time steps.

Does any of this ring any bells? I guess I should read the documentation to understand what terms you actually do assemble, unless you have the answer to the questions above ready right away.

@spco
Copy link
Contributor

spco commented Apr 7, 2016

Ah, I misunderstood! I do indeed handle everything implicitly. I think this largely was just to be in line with the fact that the convection term in the cell is implicit - please correct me if I have also misunderstood the role of current_velocity_values: I've never quite worked out how everything works with current_linearization_point!

@bangerth
Copy link
Contributor Author

bangerth commented Apr 7, 2016

On 04/07/2016 01:09 PM, Sam Cox wrote:

Ah, I misunderstood! I do indeed handle everything implicitly. I think
this largely was just to be in line with the fact that the convection
term in the cell is implicit

Ah, excellent point. Yes, we do want to treat advection implicitly.
(Although C-N is implicit as well, but I don't recall whether we do
anything of that sort for advection.) I think this clarifies things for
me. Thanks!

@bangerth
Copy link
Contributor Author

bangerth commented Apr 7, 2016

(The discussion above is related to #661, for reference.)

@bangerth bangerth deleted the avoid-warning-2 branch May 23, 2017 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants