Bug fix in non-cell centered AMR

[lroberts36]

PR Summary

On develop, there is a bug that shows up when performing AMR on non-cell centered fields. Coming out of RedistributeAndRefineMeshBlocks, FillDerived may have been called using uninitialized non-cell centered data at certain locations on newly refined blocks. Additionally, the boundaries of those non-cell centered fields may have been filled with uninitialized neighbor data at certain locations.

This PR fixes that by breaking out the different tasks in Mesh::Initialize and calling them in the correct order to ensure that non-cell centered fields are filled correctly in the interior before calling FillDerived (and PreCommFillDerived). Additionally, a second communication is added to make sure the ghost values are correctly filled. The extra communication uses mesh data that only contains the non-cell centered fields with FillGhost and does not occur if none of these fields exist.

This bug strongly suggests that we need to add a regression test that stresses non-cell centered fields, but I think that will be a bit of work and too big for this PR. As @Yurlungur suggests in #1019, a transverse vector wave equation solver may be a good choice for this test.

PR Checklist

• Code passes cpplint
• Adds a test for any bugs fixed.
• Code is formatted
• Changes are summarized in CHANGELOG.md
• CI has been triggered on Darwin for performance regression tests.
• Docs build
• (@lanl.gov employees) Update copyright on changed files

[Yurlungur]

I like to see this split up. Would also like this fix in ASAP.

[bprather]

KHARMA doesn't hit this (which is all I tested when I introduced it lol). So, I'm happy to approve the good reorg now, and if it introduces a regression for me I'll figure it out later.

However, this feels like the sort of organization thing @pgrete should weigh in on before it gets upstreamed.

[lroberts36]

KHARMA doesn't hit this (which is all I tested when I introduced it lol). So, I'm happy to approve the good reorg now, and if it introduces a regression for me I'll figure it out later.

@bprather: It would probably be good to at least make sure this doesn't break things for you before it goes in since things are a bit reorganized relative to what was there before.

[bprather]

@bprather: It would probably be good to at least make sure this doesn't break things for you before it goes in since things are a bit reorganized relative to what was there before.

Yeah, I'll be pulling this as I can, but since KHARMA's currently based on a pretty forked branch it may be a bit before I could spot any regression. So, marking that I'm happy if this goes in before I get a chance.

I think if the regression tests & parthenon-mhd work, KHARMA's unlikely to hit anything crippling from this patch (and thus isn't crucial as a data point before putting this in). And, if AMR breaks in KHARMA I'm pretty sure no one cares -- no one's using it or planning to soon, and it works in the release anyway.

[lroberts36]

This branch also passes the Riot test suite

[pgrete]

I think the refactor makes sense.
One thing I don't fully understand is why this didn't come up for cell centered fields or, in other words, were is the logic split between handling cell centered and non cell-centered fields. Is this related to the ownership model for which there's no block/buffer taking precedence?
So maybe some additional comments why this separate treatment is required would be useful for future reference.

Finally, given that we don't have non-cc fields in AthenaPK, I don't have other means to test so I'm trusting the Parthenon internal and Riot results/testing.

[lroberts36]

I think the refactor makes sense. One thing I don't fully understand is why this didn't come up for cell centered fields or, in other words, were is the logic split between handling cell centered and non cell-centered fields. Is this related to the ownership model for which there's no block/buffer taking precedence? So maybe some additional comments why this separate treatment is required would be useful for future reference.

@pgrete: The key difference with cell centered fields is that internal prolongation is a no-op for them so all cell-centered values on newly refined blocks are correctly filled after a call to ProlongateShared. Previously, Initialize (i.e. communicating boundary data and calling fill derived) was called after ProlongateShared but before ProlongateInternal. This meant that if you only had cell centered data everything worked fine, but if you had non-cell centered elements on the new fine grid that were not shared with the coarse grid they were uninitialized during boundary communication and fill derived (which obviously gives junk on the grid and in the boundaries). Now, if there are non-cell centered values, we communicate them after ProlongateShared so that shared elements respect the correct ownership. Then we call ProlongateInternal, at which point the internal values of all fields are correctly filled. Then we can safely FillDerived and communicate everything so that the interior and boundaries are correct after refinement.

Finally, given that we don't have non-cc fields in AthenaPK, I don't have other means to test so I'm trusting the Parthenon internal and Riot results/testing.

I think the real test was in parthenon-mhd (where AMR was broken before adding this fix), Riot doesn't have any non-cell centered values. Really, I was more concerned to check that I didn't do something in the re-organization that impacted cell centered fields in some unexpected way.