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

Create basic buoyancy term as node kernel, named buoyancy_density #1274

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

mbkuhn
Copy link
Contributor

@mbkuhn mbkuhn commented Jul 25, 2024

Motivation

Though the more commonly used "buoyancy_boussinesq" source term is implemented as a node kernel and therefore has no issue with hypre solvers, the more basic "buoyancy" source term is not. This source term is more common for multiphase flows, though, and it shouldn't be limited to only tpetra solvers.

Description

I set up a node kernel version of the buoyancy term and named it "buoyancy_density" (as it is density-based) to differentiate it from the other implementation. This implementation has no issues with hypre solvers. Regression tests will be valuable to check against the existing buoyancy source term -- haven't done that yet.

Additional comments

I know Wyatt has been doing a lot of work with the balanced buoyancy implementation that is not yet merged, and I hesitate to disrupt that. Prior discussion leading to this PR is in #1265.

@mbkuhn mbkuhn requested review from psakievich and wjhorne July 25, 2024 22:44
@wjhorne
Copy link
Contributor

wjhorne commented Jul 29, 2024

The code looks good to me. Are there any regression tests that use it just to make sure it has coverage? We could just modify one of the multiphase tests to use buoyancy_density, for example

@mbkuhn
Copy link
Contributor Author

mbkuhn commented Jul 29, 2024

The code looks good to me. Are there any regression tests that use it just to make sure it has coverage? We could just modify one of the multiphase tests to use buoyancy_density, for example

That's a good idea, it should be covered in the reg tests. Should I changed the droplet test? Btw, does it affect your work on the balanced buoyancy at all?

@wjhorne
Copy link
Contributor

wjhorne commented Jul 29, 2024

The droplet case would be perfect. The balanced buoyancy introduces a buoyancy_source field variable that gets dropped in instead of rho * g. It should be simple to modify what you have done to pull the field when called for and use it or rho * g as necessary. I think we get this in first and then I can rebase on top of it, putting in the small modifications that it would need as part of the balanced buoyancy PR

@mbkuhn
Copy link
Contributor Author

mbkuhn commented Jul 29, 2024

Compared the results from the updated VOFDroplet case to the current version, and the reg test passes. I still updated the gold file, you can see that the differences are very small. I haven't confirmed that it works on GPUs or anything like that, but I would expect it to, given how I mimicked the buoyancy_boussinesq implementation.

@mbkuhn mbkuhn marked this pull request as ready for review July 29, 2024 16:30
Copy link
Contributor

@psakievich psakievich left a comment

Choose a reason for hiding this comment

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

Seems fine to me. @wjhorne I'll merge if you are satisfied.

@wjhorne
Copy link
Contributor

wjhorne commented Jul 29, 2024

@psakievich Go ahead and merge. It all looks good to me.

@mbkuhn mbkuhn merged commit 0bc1fca into Exawind:master Jul 29, 2024
3 checks passed
@mbkuhn mbkuhn deleted the buoyancy_density_node_kernel branch July 29, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants