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

Bug fix in boundary communication #797

Merged
merged 12 commits into from
Dec 22, 2022
Merged

Bug fix in boundary communication #797

merged 12 commits into from
Dec 22, 2022

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Dec 2, 2022

PR Summary

While debugging Riot, Josh and I found that there was an issue in the logic for which variables to do flux correction on. Previously, there was an implicit assumption that variables that had Metadata::WithFluxes set also had Metadata::FillGhosts set. There were also potentially issues if only Metadata::FillGhosts was set. This PR fixes those issues, so that flux correction is only done for all variables with Metadata::WithFluxes while boundaries are still filled for all variables that have Metadata::FillGhosts.

@pgrete: You may want to try this with issue #659, although the appearance of that issue may have happened before the sparse boundary update.

@Yurlungur: I think this change shouldn't have any impact on how you are using ForEachBoundary in your prolongation PR, but the two probably require merging (sorry).

ps: I also made a couple of small changes to the Burger's equation benchmark to stop the linter from complaining.

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.
  • (@lanl.gov employees) Update copyright on changed files

@lroberts36 lroberts36 added the bug Something isn't working label Dec 2, 2022
@Yurlungur
Copy link
Collaborator

You may want to try this with issue #659, although the appearance of that issue may have happened before the sparse boundary update.

We may have found the source of @pgrete's bug while I was in Hamburg. It's different than this but also nasty. I will let @pgrete describe after he's done some checks to ensure we did find it.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Sorry---beat you to it. Userspace prolongation is in. Up to you to do the merge. ;)

This looks entirely reasonable.

@pgrete
Copy link
Collaborator

pgrete commented Dec 13, 2022

I'll review once the merge conflict is resolved (I want you make sure that I don't break the updated logic here).

@lroberts36
Copy link
Collaborator Author

@pgrete, the merge is done. Should be ready for review whenever you get the chance.

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

It took a while for my brain to parse the logic, but I now think that this looks good.

Out of curiosity, is there a use-case where Metadata::WithFluxes would be used without Metadata::FillGhosts?

Also, I didn't see any changes to the burgers benchmark so I assume that those changes are/were not required any more.

@pgrete pgrete enabled auto-merge (squash) December 22, 2022 17:28
@pgrete pgrete merged commit 4792897 into develop Dec 22, 2022
@lroberts36
Copy link
Collaborator Author

@pgrete: Thanks for the review.

I guess if your fluxes only depended on dependent variables, you could reconstruct only dependent variables (so they would just have FillGhosts) while independent, conserved variables would still require flux arrays (so they would just have WithFluxes set).

The burgers changes must have been fixed in another commit that got merged in before this one.

@Yurlungur Yurlungur deleted the lroberts36/fix-fluxcor branch April 14, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants