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

Fix FV operators in double null configurations #2626

Merged
merged 5 commits into from
Jan 6, 2023
Merged

Conversation

bendudson
Copy link
Contributor

The Mesh::firstY() and Mesh::lastY() functions can be used in slab and single-null tokamak geometry to determine if a cell is at a Y boundary. In double null geometries this fails because the upper target boundaries are in the middle of the Y domain.

The versions of these functions taking an X index use the communicators to determine if a point is first or last on its flux surface.

This fix replaces calls to no-argument firstY() and lastY() with calls the versions taking an X index. It also deprecates the no-argument functions as they are likely to cause similar confusion in future.

Used mesh::firstY() and mesh::lastY() rather than
mesh::firstY(xind) and mesh::lastY(xind).

Those only check the global Y index, and so don't check
for boundaries in the middle of the Y domain. This causes
a problem for double null tokamak simulations.
The versions of these functions without X index don't actually give
any useful information regarding mesh topology. They give the correct
result in many cases but not all. Instead the versions with X index
argument should be used.
@bendudson bendudson added this to the BOUT-5.0 milestone Jan 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

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

When using firstY(ix) and lastY(ix), is it also necessary to check periodicY(ix) the way D4DY4_Index() does?

bool yperiodic = mesh->periodicY(i);
bool has_upper_boundary = !yperiodic && mesh->lastY(i);
bool has_lower_boundary = !yperiodic && mesh->firstY(i);

FV::Div_par does something similar

if (!mesh->firstY(i) || mesh->periodicY(i)) {

There was something in the comments for firstY(ix)/lastY(ix) about just being the first/last in the communicator?

@@ -325,7 +325,7 @@ Field3D Div_a_Grad_perp(const Field3D& a, const Field3D& f) {
result(i, j, k) += flux / (coord->J(i, j, k) * coord->dy(i, j, k));
result(i, j + 1, k) -= flux / (coord->J(i, j + 1, k) * coord->dy(i, j + 1, k));

if(j == mesh->ystart && (!mesh->firstY())) {
if(j == mesh->ystart && (!mesh->firstY(i))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this function not need any check for the upper boundary (mesh->lastY(i)), when it does for the lower boundary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @johnomotani ! I've rewritten this function to be more consistent in handling the boundaries

bendudson and others added 2 commits January 6, 2023 12:38
Thanks to @johnomotani! The periodicY(x) value should be checked
when using firstY / lastY
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@bendudson bendudson merged commit 061943e into next Jan 6, 2023
@bendudson bendudson deleted the fix-div-k-par branch January 6, 2023 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants