illinois-ceesd / mirgecom

MIRGE-Com is the workhorse simulation application for the Center for Exascale-Enabled Scramjet Design at the University of Illinois.
Other
11 stars 19 forks source link

Add radiation #893

Closed majosm closed 1 year ago

majosm commented 1 year ago

Adds radiation sink term to fluid-wall coupling.

Questions for the review:

tulioricci commented 1 year ago

@majosm , two comments:

1) should we standardize the nomenclature of BCs? Right now, this is what we have: IsothermalSlipWallBoundary IsothermalWallBoundary AdiabaticSlipBoundary AdiabaticNoslipWallBoundary

2) replace "wall_epsilon" with "wall_emissivity"

majosm commented 1 year ago
  1. replace "wall_epsilon" with "wall_emissivity"

Done (forgot to push 🙂).

MTCam commented 1 year ago

@majosm , two comments:

  1. should we standardize the nomenclature of BCs? Right now, this is what we have: IsothermalSlipWallBoundary IsothermalWallBoundary AdiabaticSlipBoundary AdiabaticNoslipWallBoundary

I dislike the disparity in naming, too. I also dislike when every BC has "WallBoundary" attached to it, if they all have it, then why have it at all? For me, these could easily be IsothermalSlip, IsothermalNoslip , etc. It's already known as a BC because we from mirgecom.boundary import IsothermalNoslip.

Now that we have multiple domains and materials, we could re-think some of the code structuring too, like renaming mirgecom.boundary to mirgecom.fluid_boundary. But again, these are all non-essential changes that can easily distract us and seem unimportant unless we are already set up to really nail the prediction.

Edit: also out of scope for this pr

tulioricci commented 1 year ago

And we don't need to get the gradient here (https://github.com/illinois-ceesd/mirgecom/blob/d512ebd2a0690a9ad1c5b1581ac59c360bee34bf/mirgecom/multiphysics/thermally_coupled_fluid_wall.py#L1839) because it is already computed above

majosm commented 1 year ago

And we don't need to get the gradient here (

https://github.com/illinois-ceesd/mirgecom/blob/d512ebd2a0690a9ad1c5b1581ac59c360bee34bf/mirgecom/multiphysics/thermally_coupled_fluid_wall.py#L1839

) because it is already computed above

It's not actually being recomputed there, since we're passing it in as grad_u (and similarly for the fluid side). So basically it's just cosmetic. I tried removing it (code here), but I don't know if it's any cleaner. 🤷‍♂️ Thoughts?

tulioricci commented 1 year ago

And we don't need to get the gradient here ( https://github.com/illinois-ceesd/mirgecom/blob/d512ebd2a0690a9ad1c5b1581ac59c360bee34bf/mirgecom/multiphysics/thermally_coupled_fluid_wall.py#L1839

) because it is already computed above

It's not actually being recomputed there, since we're passing it in as grad_u (and similarly for the fluid side). So basically it's just cosmetic. I tried removing it (code here), but I don't know if it's any cleaner. man_shrugging Thoughts?

I prefer the new version, but it is not a big deal since, in practice, we may end up having this in the driver itself... :man_shrugging:

tulioricci commented 1 year ago

LGTM, but I will let @MTCam give the final word.