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 bug in static refinement #1071

Merged
merged 10 commits into from
May 15, 2024
Merged

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented May 9, 2024

PR Summary

The re-definition of Mesh::root_level introduced in #1053 did not get correctly propagated through the static refinement related code (which operates on logical locations relative to the legacy tree). This PR fixes that issue, which had cropped up in Artemis.

PR Checklist

  • Make AMR and sparse restarts bitwise exact #1073 is merged
  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

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.

Thanks for the catch! To prevent this kind of thing from happening again... maybe we should have a regression test that uses SMR?

@lroberts36
Copy link
Collaborator Author

Agreed, we need to add a couple tests that use multi-tree forests. We have at least one that uses SMR, but the bug does not manifest if the base grid only requires one tree.

@lroberts36 lroberts36 changed the title WIP: Fix bug in static refinement Fix bug in static refinement May 9, 2024
@lroberts36 lroberts36 requested review from brryan, pgrete and jdolence May 13, 2024 18:22
@lroberts36 lroberts36 added the bug Something isn't working label May 13, 2024
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.

👍 for fixes and 👍 for tests

Copy link
Collaborator

@pdmullen pdmullen left a comment

Choose a reason for hiding this comment

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

LGTM

@lroberts36 lroberts36 enabled auto-merge May 14, 2024 17:16
@lroberts36 lroberts36 disabled auto-merge May 14, 2024 17:23
@lroberts36 lroberts36 changed the base branch from develop to lroberts36/fix-restart-state-bug May 14, 2024 17:38
@lroberts36 lroberts36 mentioned this pull request May 14, 2024
7 tasks
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.

LGTM (plus optional comments on the python stuff)

The updated gold files are in #1073, right?

+ "/tst/regression/gold_standard/sparse_twotree.out0.final.phdf",
],
one=True,
tol=1e-12,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for the "loose" tolerance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I just copied this tolerance from the other checks. Isn't 1.e-12 what we set for most tests? I don't think this test should be bitwise exact between machines.

@lroberts36
Copy link
Collaborator Author

The updated gold files are in #1073, right?

Yeah, I added the new gold files required here (and in #1047) to v23 so that I didn't have to upgold a bunch of times.

@lroberts36 lroberts36 changed the base branch from lroberts36/fix-restart-state-bug to develop May 15, 2024 19:53
@lroberts36 lroberts36 changed the base branch from develop to lroberts36/fix-restart-state-bug May 15, 2024 20:19
@lroberts36 lroberts36 changed the base branch from lroberts36/fix-restart-state-bug to develop May 15, 2024 20:20
@lroberts36 lroberts36 enabled auto-merge May 15, 2024 20:21
@lroberts36 lroberts36 merged commit 8bb2ece into develop May 15, 2024
49 checks passed
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.

4 participants