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.

Sign up for GitHub

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

Jump to bottom

Feature: Auxiliary scalars #467

Open
wants to merge 13 commits into
base: development
Choose a base branch
from
Open

Feature: Auxiliary scalars #467

wants to merge 13 commits into from

Conversation

ThomasHowarth
Copy link
Contributor

@ThomasHowarth ThomasHowarth commented Jan 17, 2025 •
edited
Loading

This pull request may close #465

Auxiliary variables are added through the following flags in the input:

peleLM.aux_vars = a b c

peleLM.a.advect = 1 
peleLM.a.diff_coeff = 0

peleLM.b.advect = 0
peleLM.b.diff_coeff = 1e-2

peleLM.c.advect = 1
peleLM.c.diff_coeff = 1e-2

where the advect parameter determines whether or not the variable is transported, and the diff_coeff provides a diffusion coefficient.

The test case transports/diffuses a Gaussian profile through a periodic domain. For a 1m square domain, with 1m/s upward velocity, I see the following:

  • $$t=0$$:
    image
  • Scalar A (advected, not diffused):
    • $$t=0.5$$:
      image
    • $$t=1$$:
      image
  • Scalar B (not advected, diffused):
    • $$t=0.5$$
      image
    • $$t=1$$
      image
  • Scalar C (Advected and diffused):
    • $$t=0.5$$
      image
    • $$t=1$$
      image

Discussion points:

  1. User-defined function interfaces in the setup file (i.e. pelelmex_initdata and bcnormal) have been adjusted. I've done this to all cases, but when people pull this, their current cases may not compile. Not sure how to address this.
  2. The implementation is done via the SDC iterations and is therefore not done in the case of peleLM.incompressible = 1
  3. Currently the diffusion coefficient is specified as an input, rather than linked to a particular species/temperature. It should be relatively easy to link the coefficient to it since it is set in basically the same place, but I'm not sure what the choice should be.
  4. Currently there is no source term implementation (e.g. for an age equation), however, when it is done, I suppose this could be integrated with @dmontgomeryNREL 's ODEQty in some fashion.
  5. When filling the boundary conditions, the s_aux array passed to bcnormal needs to be hardcoded. I've set this to NVAR and put a check to make sure that m_nAux < NVAR, but perhaps there is a more elegant way to do this.

@ThomasHowarth ThomasHowarth marked this pull request as draft January 17, 2025 12:55
@ThomasHowarth ThomasHowarth marked this pull request as ready for review January 20, 2025 08:51
@baperry2
Copy link
Collaborator

Regarding your discussion points:

  1. I don't like making changes that break things for users, but sometimes that can't be avoided. We should take a bit of extra time to make sure any interface changes are what we want and there aren't additional related changes we can do at the same time so that we do this as rarely as possible.
  • I don't believe it's necessary to add nAux to pelelmex_initdata: instead the user could use aux.nComp() (an Array4 member function) to get this information as needed without modifying the interface.
  • We could potentially avoid adding s_aux to bcnormal by stacking the aux data at the end of s_ext (which is just a pointer to AMReX::Real), but that's ugly and unintuitive, so a breaking change for the interface might be the lesser of two evils here.
  • We have some other potential breaking changes in the pipeline to enable inflows on EBs (Developments shopping list #156). It may make sense to delay merging this until those are done to minimize the number of times we break things for users
  1. That's ok with me for now - essentially all real use cases for LMeX are not incompressible anyway.
  2. I think it makes sense to follow what we do when we have constant non-unity Le transport. Let users specify a Schmidt number (or Lewis/Prandtl) and define the scalar diffusion coefficients based on that Sc and the viscosity.
  3. Yes, after merging this we will revisit the ODEQty stuff and fold it into this framework.
  4. This is difficult because we need a copy of the Aux values so they don't get overwritten by calls to the bcnormal function that are applying BCs for other variables. We might not be able to avoid something akin to your current approach. But an improvement would be to create a new macro like PELELMEX_MAX_NAUX that can be set in the GNUmakefile and cmake commands rather than pinning the max to NVAR - allowing users to have more AUX variables for small mechanisms and not having so much unnecessary data allocated when NVAR is large. @marchdf - any ideas for a better solution when needing dynamically sized memory within a device function?

As always, I appreciate the new capability, which I'm sure will be quite useful to many.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baperry2 baperry2 Awaiting requested review from baperry2

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Transporting passive scalars
3 participants
@ThomasHowarth @baperry2 @terencelehmann