Code review comment for lp:~ctjacobs-multiphase/fluidity/compressible-multiphase

Revision history for this message
Stephan Kramer (s-kramer) wrote :

Hey there. I've only looked at Advection_Diffusion_CG.F90 yet. Looking great so far, the maths of the changes in the i.e. equation seem to make sense to me. Here it goes:

* Currently the multiphase logical is only set within the
equation_type==FIELD_EQUATION_INTERNALENERGY case. Can we either:
- always set multiphase to be true or false, or
- rename it to mean multiphase_internal_energy_equation
As it is now suggested that multiphase can be used independently of
the value of equation_type.

* Just being picky here: can we get rid of the nullify(vfrac) in line 506. It seems
to suggest that vfrac is used later on, which it's not.

* Advection_Diffusion_CG.F90, line 517:

Why does the behaviour change if we're not multiphase? Do you maybe mean:

if (.not. multiphase .or. have_option(....compressible) then

* Advection_Diffusion_CG.F90, line 794 having an allocate within an element
loop is not ideal. It's not efficient, and it's also not thread-safe. I can
see the problem with following the usual approach (nvfrac might
not be defined), but I think it's better to do some kludge with allocating
a dummy nvfrac or something rather.

* Advection_Diffusion_CG.F90, lines 1492 and 1502: I don't think you intended to
add (istate) there.

* Not too excited about having to pass in an array of states
to advection_diffusion_cg now tbh. One way to keep the damage limited would be
to move the call to add_heat_transfer up one level inside
solve_field_equation_cg. And keep everything else just limited to a
single state, i.e. not passing istate all the way down and changing state ->
state(istate) *everywhere*.

« Back to merge proposal