search

MPAS-Dev / MPAS-Model

Repository for MPAS models and shared framework releases.
235 stars 312 forks source link

Fix halo exchange unit tests in the test core #1135

Closed jim-p-w closed 7 months ago

jim-p-w commented 7 months ago

Change test validation code to correctly evaluate halo exchange results.

The initial test code which validates halo exchanges incorrectly uses every element in the variable arrays when looking for errors. All of the variable array sizes are one greater than the actual data contents, e.g. cell variable array sizes are nCells+1, edge variable array sizes are nEdges+1, etc.

After the halo exchange, valid elements to be checked range between 1:nCells for cells, 1:nEdges for edge variables, etc.

mgduda commented 7 months ago

From the PR description I understand the issue and how it might be fixed. I agree that the changes in this PR fix the issue, but perhaps it would be better to separate the fix itself from the refactoring? Here's an example of what a minimal set of changes to address the issue might look like: https://github.com/mgduda/MPAS-Model/commit/c885cd93 .

jim-p-w commented 7 months ago

From the PR description I understand the issue and how it might be fixed. I agree that the changes in this PR fix the issue, but perhaps it would be better to separate the fix itself from the refactoring? Here's an example of what a minimal set of changes to address the issue might look like: mgduda@c885cd93 .

Your proposed fix is definitely more concise than the refactoring I proposed. Should I close this PR?

mgduda commented 7 months ago

@jim-p-w No need to close this PR. I'm only suggesting that it would be nice to separate the minimal fix for the issue from the refactoring. As it is right now with refactoring, there's a net reduction in the number of lines of code, which is always nice.

So, I'd propose updating this PR so that there are two commits, the first of which fixes the issue, and the second of which refactors the halo exchange unit test code.