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

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

Some final, small remarks:

* assemble/Divergence_Matrix.F90: is there a reason you're not using state%option_path? I prefer that as it makes future redesigns of the optionstree easier.

* assemble/Momentum_Equation.F90, line 471: does this comment still apply? if not, how is this fixed?

* assemble/Momentum_Equation.F90, line 1049: why do we all of a sudden extract density from state there? AFAIK Density is not a required field, for instance for Boussinesq. Similar for line 1585: does this not break if we don't have
a Density, but do have a prognostic Velocity and Pressure?

* assemble/Multiphase.F90, add_fluid_particle_drag_element(): doing string comparisons at the element level is not ideal. In particular with OPTION_PATH_LEN (8192 or so?) length string, the compiler might do something
sensible, but I'm not sure and I don't like second-guessing. A solution would be to move the select case up one level, and set an integer with some "enum"-like parameters DRAG_CORRELATION_TYPE_ERGUN=3, etc.

* assemble/Multiphase.F90, line 625 and 632: again here, why not use state%option_path?

* assemble/Multiphase.F90, add_heat_transfer(): looks great and I'm sure is 100% correct - good to see it's well tested though!

Great stuff, well documented, lots of tests. Next time, could we maybe merge this in smaller bits?

« Back to merge proposal