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

Fix the calculation of divu at inflow face in nodal projection so that it does not use tangential velocities on an inflow face #1219

Merged
merged 4 commits into from
Aug 2, 2020
Merged

Fix the calculation of divu at inflow face in nodal projection so that it does not use tangential velocities on an inflow face #1219

merged 4 commits into from
Aug 2, 2020

Conversation

asalmgren
Copy link
Member

Summary

Mathematically the nodal projection should only see the normal velocity at inflow faces, but it was coded so that it would use non-zero tangential velocities at inflow in the calculation of divu. Most users never set tangential velocities to anything but zero, but if they did, this would give an incorrect divergence.

Additional background

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • are described in the proposed changes to the AMReX documentation, if appropriate

asalmgren added 4 commits July 31, 2020 12:52
@asalmgren
Don't use tangential velocity at inflow face in the calculation of th…
fb83b8d 
…e divergence --

this first commit does only 3D with EB.
@asalmgren
Add same changes to divu_eb in 2d as previously in 3d
09d34ac
@asalmgren
Don't use tangential velocity at inflow face in the calculation of th…
02d7747 
…e divergence --

    this first commit does the non-EB divergence routines for 2D and 3D
@asalmgren
Add arguments to 1d divu functions so we can leave the
7d7004e 
interface dimension-independent -- note these aren't used
since there is no tangential velocity in 1d
@WeiqunZhang
Copy link
Member

In IAMR (https://github.com/AMReX-Codes/IAMR/blob/master/Source/Projection.cpp#L2353), MAESTROeX (https://github.com/AMReX-Astro/MAESTROeX/blob/main/Source/MaestroNodalProj.cpp#L495), and incflo (https://github.com/AMReX-Codes/incflo/blob/main/src/projection/incflo_apply_nodal_projection.cpp#L139), we already set velocity in ghost cells to zero except for inflow boundary. I wonder if we could consolidate them. Maybe we could zero out velocities at the beginning of compRHS in AMReX_MLNodeLaplacian.cpp so that application codes and other parts of nodal solver do not need to do it.

@asalmgren
Copy link
Member Author

asalmgren commented Aug 1, 2020 via email

@jbbel
Copy link
Contributor

jbbel commented Aug 1, 2020 via email

@WeiqunZhang
Copy link
Member

I understand that. The code in incflo currently looks like this

- fillpatch velocity
- do advection and diffusion
- do projection

In do pojection,

- zero out velocity in ghost cells, but not the tangential velocity  # let call call this step `zeroVelocity`
- call mlmg

In the PR, mlmg avoids using tangential velocity. What I am trying to say is since we are touching the ghost cells before the solve anyway, we could set tangential velocity to zero when we are in the zeroVelocity step. And since the zeroVelocity step is done in all codes, we could move it to AMReX. Note that when the velocity is used in next advection/diffusion, fillpatch will be called again. So setting ghost cells to zero does not affect advection and diffusion.

@WeiqunZhang
Copy link
Member

As Ann mentioned we handle wall correctly. We handle the wall by setting ghost cells to zero. Here we handle inflow tangential velocity by if tests. What I am suggesting we could handle them the same way by setting inflow tangential velocity to zero.

@drummerdoc
Copy link
Member

So, I guess I'm getting confused. It seems like this PR sets up the nodal projection in AMReX to avoid using tangential velocities in the projection, and Weiqun is suggesting that AMReX avoids using the tangential velocities in the projection, which sorta sounds exactly the same. Moreover, both are suggesting that user BC routines allow for inflowing tangential velocities (as is required for advection and diffusion).

@asalmgren
Copy link
Member Author

asalmgren commented Aug 2, 2020 via email

@drummerdoc
Copy link
Member

Thank you, that helps a ton.

drummerdoc
drummerdoc reviewed Aug 2, 2020
View reviewed changes
Copy link
Member

@drummerdoc drummerdoc left a comment

Choose a reason for hiding this comment

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

I agree that it is better to leave the terms out of the stencil than to count on them being zeroed by the caller. This looks fine to me.

@asalmgren
Copy link
Member Author

asalmgren commented Aug 2, 2020 via email

@WeiqunZhang
Copy link
Member

I looked more carefully at IAMR (https://github.com/AMReX-Codes/IAMR/blob/master/Source/Projection.cpp#L2353). IAMR does zero out tangential velocity at inflow face.

@asalmgren
Copy link
Member Author

asalmgren commented Aug 2, 2020 via email

@WeiqunZhang WeiqunZhang merged commit ccaebcc into AMReX-Codes:development Aug 2, 2020
@asalmgren asalmgren deleted the nodal_inflow_fix branch August 13, 2020 01:07
dwillcox pushed a commit to dwillcox/amrex that referenced this pull request Oct 3, 2020
@asalmgren @dwillcox
Fix the calculation of divu at inflow face in nodal projection so tha…
55f3f7c 
…t it does not use tangential velocities on an inflow face (AMReX-Codes#1219)

## Summary
Mathematically the nodal projection should only see the normal velocity at inflow faces, but it was coded so that it would use non-zero tangential velocities at inflow in the calculation of divu.  Most users never set tangential velocities to anything but zero, but if they did, this would give an incorrect divergence.
## Additional background

## Checklist

The proposed changes:
- [X] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [X] changes answers in the test suite to more than roundoff level
- [X] are likely to significantly affect the results of downstream AMReX users
- [ ] are described in the proposed changes to the AMReX documentation, if appropriate
@sayerhs sayerhs mentioned this pull request Nov 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drummerdoc drummerdoc drummerdoc left review comments

@WeiqunZhang WeiqunZhang WeiqunZhang approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@asalmgren @WeiqunZhang @jbbel @drummerdoc