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

Set the correct root level on restart #1053

Merged
merged 6 commits into from
Apr 17, 2024
Merged

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Apr 15, 2024

PR Summary

Currently, on restart Mesh::root_level and Mesh::current_level get set to levels relative to the legacy tree, which is incorrect (see issue #1051). This PR fixes the issue.

PR Checklist

  • Code passes cpplint
  • 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 requested review from pgrete and Yurlungur April 15, 2024 19:09
@lroberts36 lroberts36 linked an issue Apr 15, 2024 that may be closed by this pull request
@lroberts36 lroberts36 changed the title Set the correct root level on restart WIP: Set the correct root level on restart Apr 15, 2024
@lroberts36 lroberts36 changed the title WIP: Set the correct root level on restart Set the correct root level on restart Apr 15, 2024
@lroberts36 lroberts36 added the bug Something isn't working label Apr 15, 2024
@lroberts36 lroberts36 enabled auto-merge April 15, 2024 20:10
@lroberts36 lroberts36 disabled auto-merge April 15, 2024 20:10
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.

LGTM

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.

Alright, this explains some of the issues I've been seeing.

With current develop I got

Number of physical refinement levels = 2
Number of logical  refinement levels = 4

for a sample restart from the input file of the advection example.
With this branch I get the

Number of physical refinement levels = 2
Number of logical  refinement levels = 2

that matches the original run.

So just double checking: The GetLegacyRootLevel call for the output is correct, isn't it?

@pgrete
Copy link
Collaborator

pgrete commented Apr 16, 2024

Also what worries me is that we didn't catch this with the regression tests (particularly as current develop segfaults on restart with the advection example...).
Did this just pass by chance?

@lroberts36
Copy link
Collaborator Author

@pgrete: GetLegacyRootLevel for output is correct for as long as we are outputting legacy locations. This PR just ignores that value on restart and gets the new root level from the forest when it is rebuilt.

I am a little surprised develop segfaulted for restarts. When I was working on the forest PR, I tested restarting sparse advection with non-trivial forests and it seemed to work. That being said, I agree we might need to make the restart test more robust.

@lroberts36 lroberts36 merged commit b2fb321 into develop Apr 17, 2024
49 checks passed
@lroberts36 lroberts36 mentioned this pull request May 9, 2024
13 tasks
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.

pmb->loc.level() differences in writing and reading restart files
3 participants