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

Attempt at radiation as a pseudo-boundary condition #855

Closed
wants to merge 16 commits into from

Conversation

tulioricci
Copy link
Collaborator

Not meant for merging.

Questions for the review:

  • Is the scope and purpose of the PR clear?
    • The PR should have a description.
    • The PR should have a guide if needed (e.g., an ordering).
  • Is every top-level method and class documented? Are things that should be documented actually so?
  • Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
  • Does the implementation do what the docstring claims?
  • Is everything that is implemented covered by tests?
  • Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

@tulioricci tulioricci added the nomerge Not meant for merging label Mar 24, 2023
@majosm majosm changed the base branch from production to production-radiation March 24, 2023 19:51
Copy link
Collaborator

@majosm majosm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look through and added some thoughts.

Comment on lines +103 to +104
x_plus_y = actx.np.where(actx.np.greater(x + y, 0.0*x), x + y, 0.0*x+1.0)
return 2.0*x*y/x_plus_y
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Coefs are allowed to be integers AFAIK. 🙂)

Comment on lines +277 to +334
class PrescribedFluxDiffusionBoundary(DiffusionBoundary):
r"""Prescribed flux boundary condition for the diffusion operator.

For the boundary condition $(\nabla u \cdot \mathbf{\hat{n}})|_\Gamma$, uses
external data

.. math::

u^+ = u^-

when computing the boundary fluxes for $\nabla u$, and applies directly
the prescribed flux when computing $\nabla \cdot (\kappa \nabla u)$. This
is not the same as prescribing the gradient of $u$.

.. automethod:: __init__
.. automethod:: get_grad_flux
.. automethod:: get_diffusion_flux
"""

def __init__(self, bnd_func):
"""
Initialize the boundary condition.

Parameters
----------
bnd_func:
function to prescribe $g$ along the boundary
"""
self._function = bnd_func

def get_grad_flux(self, dcoll, dd_bdry, kappa_minus, u_minus): # noqa: D102
actx = u_minus.array_context
kappa_tpair = TracePair(
dd_bdry, interior=kappa_minus, exterior=kappa_minus)
u_tpair = TracePair(
dd_bdry, interior=u_minus, exterior=u_minus)
normal = actx.thaw(dcoll.normal(dd_bdry))
# XXX using the kappa-weighted average.
return weighted_grad_facial_flux(kappa_tpair, u_tpair, normal)

def get_diffusion_flux(
self, dcoll, dd_bdry, kappa_minus, u_minus, grad_u_minus,
lengthscales_minus, *, penalty_amount=None, **kwargs): # noqa: D102
actx = u_minus.array_context
normal = actx.thaw(dcoll.normal(dd_bdry))
kappa_tpair = TracePair(
dd_bdry, interior=kappa_minus, exterior=kappa_minus)
u_tpair = TracePair(
dd_bdry, interior=u_minus, exterior=u_minus)
grad_u_tpair = TracePair(
dd_bdry, interior=grad_u_minus, exterior=grad_u_minus)
lengthscales_tpair = TracePair(
dd_bdry, interior=lengthscales_minus, exterior=lengthscales_minus)
# Note the flipped sign here to match the diffusion notation
return - self._function(kappa_tpair, u_tpair, grad_u_tpair,
lengthscales_tpair, normal, penalty_amount, **kwargs) + u_minus*0.0


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't being used at the moment, right?

Comment on lines +140 to +143
# FIXME this function is already in "diffusion"
def _harmonic_mean(actx, x, y):
x_plus_y = actx.np.where(actx.np.greater(x + y, 0.0*x), x + y, 0.0*x+1.0)
return 2.0*x*y/x_plus_y
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could put this in mirgecom.math, maybe?

Comment on lines +192 to +193
# TODO move this to "diffusion" ???
def _diffusion_facial_flux_upwind_with_radiation(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is kind of an interface-specific/heat-specific thing (whereas diffusion_operator itself is more physics-agnostic), I think it's probably best to leave this here. We should probably rename it though, since it's not really "upwind" anymore. _diffusion_facial_flux_central_with_radiation?

Comment on lines +211 to +220
# FIXME not entirely sure about this. Need to keep an eye on it...
# Computing a centered flux "int + ext - radiation"
# TRR: I disagree about the signs here, but that may be because of the
# normal orientation...
flux_on_solid_side = \
np.dot(diffusion_flux(kappa_tpair.int, grad_u_tpair.int), normal)
flux_on_fluid_side = (
np.dot(diffusion_flux(kappa_tpair.ext, grad_u_tpair.ext), normal)
+ epsilon_minus * sigma * u_tpair.int**4)
flux_without_penalty = 0.5*(flux_on_solid_side + flux_on_fluid_side)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense for the radiation term to be +, since the normal faces outward.

mom_bc = self._slip.momentum_bc(cv_minus.momentum, normal)
t_minus = state_minus.temperature
t_plus = _project_from_base(dcoll, dd_bdry, self._t_plus)
t_bc = (t_minus + t_plus)/2.0 # XXX Maybe use t_bc as t_plus?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah seems like it would make sense to use t_plus here.

Comment on lines +758 to +764
def grad_temperature_bc(self, dcoll, dd_bdry, gas_model, state_minus,
grad_cv_minus, grad_t_minus, **kwargs):
"""Get grad(T) on the boundary uses the internal/minus value (aka fluid).

This is the same logic of an isothermal BC.
"""
return _project_from_base(dcoll, dd_bdry, grad_t_minus)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just omit this function like IsothermalWallBoundary does. (PrescribedFluidBoundary defaults to this behavior.)

dcoll, dd_bdry, kappa_minus, state_minus.temperature)
t_minus = state_minus.temperature
t_plus = _project_from_base(dcoll, dd_bdry, self._t_plus)
t_bc = (t_minus + t_plus)/2.0 # XXX Maybe use t_bc as t_plus?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Same comment as the other instance.)

kappa_plus = _project_from_base(dcoll, dd_bdry, self.kappa_plus)
kappa_tpair = TracePair(
dd_bdry, interior=kappa_minus, exterior=kappa_plus)

u_plus = _project_from_base(dcoll, dd_bdry, self.u_plus)
u_tpair = TracePair(dd_bdry, interior=u_minus, exterior=u_plus)

from mirgecom.diffusion import grad_facial_flux
return grad_facial_flux(kappa_tpair, u_tpair, normal)
return average_grad_facial_flux(kappa_tpair, u_tpair, normal)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking this should only use kappa_plus/t_plus (or do the 2*t_plus - t_minus thing to achieve the same effect).

@@ -1429,6 +1431,7 @@ def coupled_ns_heat_operator(
dcoll, fluid_all_boundaries, fluid_state, quadrature_tag=quadrature_tag,
dd=fluid_dd, **av_kwargs)

# XXX is sigma necessary here?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think sigma isn't needed inside diffusion_operator, as it's only used in the boundary (which gets constructed externally and passed in).

@tulioricci
Copy link
Collaborator Author

Superseded by #893.

@tulioricci tulioricci closed this May 1, 2023
@tulioricci tulioricci deleted the tulio/prod-radiation branch July 17, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nomerge Not meant for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants