search

MPAS-Dev / MPAS

Repository for private MPAS development prior to the MPAS v6.0 release.
Other
4 stars 0 forks source link

Correct the sign of edgeNormalVectors for positive inflow boundary edges #1512

Closed mgduda closed 6 years ago

mgduda commented 6 years ago

This merge corrects the sign of edgeNormalVectors for positive inflow boundary edges.

For edges that represent positive normal inflow along the boundary of a mesh, i.e., for edges where (cellsOnEdge(1,iEdge) == nCells+1), the mpas_initialize_vectors subroutine had incorrectly computed the value of the edgeNormalVectors field due to the transposition of the iEdge and cell2 locations in the difference.

mgduda commented 6 years ago

@xylar and @matthewhoffman We've tested these changes for non-periodic meshes, but if either of you would be able to easily test this PR with a periodic mesh, that would be really helpful.

xylar commented 6 years ago

@mgduda, I'll see if I can find a good test case on our side and will review this next week.

xylar commented 6 years ago

I agree with @mark-petersen. We have periodic meshes, some of which have missing cells (because they have been culled) but we never have fluxes at those boundary edges so we would not have a way of seeing that this bug has been fixed in MPAS-O.

xylar commented 6 years ago

@matthewhoffman, do you happen to have a mesh that is periodic in one dimension and has been culled in the other dimension? Do any MPAS-LandIce tests have fluxes at boundary edges?

xylar commented 6 years ago

@mgduda, I took a look at a periodic grid that includes cell culling in MPAS-O. It seems the cell-culler tool we use always makes sure that cell1 = cellsOnEdge(1,iEdge) is the valid cell index so that cell2 = cellsOnEdge(2,iEdge) would be equal to nCells+1 for a boundary edge. This means that we never encounter the case cell1 == nCells+1 in any of our meshes.

Here are a couple of images from a mesh (from the sub_ice_shelf_2D COMPASS test case) that show this: edge_normal_periodic1 edge_normal_periodic2

So I think you can feel free to merge this PR. We don't have a way to create a mesh where cell1 == nCells+1 .and. is_periodic, and it seems like too much work to go out of our way to create such a mesh just to test this PR.

mgduda commented 6 years ago

@matthewhoffman Did you have any comments or concerns about this PR?

matthewhoffman commented 6 years ago

@mgduda , we do have tests we can modify to be periodic, so I spent some time setting that up, and then realized that we don't actually have any code that makes use of edgeNormalVectors, so I wasn't actually testing anything. So go ahead and merge this.