-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add compressibility to heat transfer #1051
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! The only thing I would change is the name of the dof handler. Maybe just use "fluid_dynamics_cell"?
@@ -454,8 +454,10 @@ HeatTransfer<dim>::assemble_local_system_matrix( | |||
const DoFHandler<dim> *dof_handler_fluid = | |||
multiphysics->get_dof_handler(PhysicsID::fluid_dynamics); | |||
|
|||
typename DoFHandler<dim>::active_cell_iterator velocity_cell( | |||
&(*triangulation), cell->level(), cell->index(), dof_handler_fluid); | |||
typename DoFHandler<dim>::active_cell_iterator fd_cell(&(*triangulation), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is fd meant to mean "fluid dynamics"? Maybe I would use a more meaningful name here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the fd here to be honest. All the fluid dynamics functions have fd, heat transfers have ht, etc. Right now this is a standard nomenclature in the code, so I would not change that unless we change it everywhere and that owuld be another PR in itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think making a second reinit function is a good pattern here. It would be a better idea to reinit everything related to a single physics in a single function. Otherwise you will have multiple calls to fe_values.reinit and that's expensive.
* components and pressure values. | ||
*/ | ||
template <typename VectorType> | ||
void | ||
reinit_pressure(const typename DoFHandler<dim>::active_cell_iterator &cell, | ||
const VectorType ¤t_solution) | ||
{ | ||
this->fe_values_fd.reinit(cell); | ||
this->fe_values_fd[pressure].get_function_values(current_solution, | ||
pressure_values); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait why is this a second FE_values reinit? This is very expensive as a call. It would be better to gather pressure when we gather the fluid dynamics instead of calling reinit when we need both...
source/solvers/heat_transfer.cc
Outdated
scratch_data.reinit_pressure(fd_cell, | ||
*multiphysics->get_block_solution( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to move all the reinits to a single reinit which would be reinit_fluid_dynamics,. Otherwise the call to fe_values reinit will get prohibitively expensive..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I'll do so tomorrow!
I'll also check if it is more costly to add an if
inside to get the pressure values or if the get_function_values()
is not to bad. If it is not too bad (which I don't think is the case), I'll leave it without the if
is that okay ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup sounds good
Co-authored-by: Bruno Blais <[email protected]>
Will merge after CI @AmishgaAlphonius |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job Amish! Everything fine on my side.
Description of the problem The previously implemented weak compressibility () was not working with heat transfer physic. Description of the solution Missing pressure field was added to scratch data. How Has This Been Tested? The new feature has been tested on an impinging jet case. Two application tests were added to test this feature : applications_tests/lethe-fluid/heat_transfer_weakly_compressible_flow.prm (.output) applications_tests/lethe-fluid/heat_transfer_vof_weakly_compressible_flow.prm (.output) Future changes Pressure field should be added to other scratch data using the density. To generalize, all fields should be checked if required in all scratch data that calculates physical properties. Comments Other small typo corrections were made. Co-authored-by: Bruno Blais <[email protected]> Former-commit-id: 1b47e0e
Description of the problem The previously implemented weak compressibility () was not working with heat transfer physic. Description of the solution Missing pressure field was added to scratch data. How Has This Been Tested? The new feature has been tested on an impinging jet case. Two application tests were added to test this feature : applications_tests/lethe-fluid/heat_transfer_weakly_compressible_flow.prm (.output) applications_tests/lethe-fluid/heat_transfer_vof_weakly_compressible_flow.prm (.output) Future changes Pressure field should be added to other scratch data using the density. To generalize, all fields should be checked if required in all scratch data that calculates physical properties. Comments Other small typo corrections were made. Co-authored-by: Bruno Blais <[email protected]> Former-commit-id: 1b47e0e
Description of the problem
Description of the solution
How Has This Been Tested?
applications_tests/lethe-fluid/heat_transfer_weakly_compressible_flow.prm
(.output
)applications_tests/lethe-fluid/heat_transfer_vof_weakly_compressible_flow.prm
(.output
)Future changes
fields
should be checked if required in all scratch data that calculates physical properties.Comments