-
Notifications
You must be signed in to change notification settings - Fork 37
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 reading restarts due to hidden ghost var #1104
Conversation
PARTHENON_HDF5_CHECK( | ||
H5Sselect_hyperslab(hdl.dataspace, H5S_SELECT_SET, offset, NULL, count, NULL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lroberts36 found that this line is duplicated two lines below, so I cleaned it up (independent of the bugfix in this PR)
I don't understand how the CI passed with this bug...? Also: would it make sense to add an action to run the CI with ASAN enabled to catch unitialized value bugs in general? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are a godsend @pgrete. I lost my day to this bug yesterday... I couldn't track it down for the life of me. Confirmed that this fixes a restart issue encountered with another parth downstream code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice catch!
I think that
I agree that this would be a good idea. |
PR Summary
hasGhost
was defined both inrestart.hpp
andrestart_hdf5.hpp
We only set the value inside the constructor of the derived class but in the parthenon manager we work with the base class.
The latter actually set the data based on bounds defined by
hasGhosts
so it made a decision based on uninitialized data.On Frontier I could reliably reproduce the issue (both with cray and amdclang/hip compilers), whereas on other machines/compilers
hasGhosts
was set by default/accident to0
in the base class (though also rendering reading restarts with ghosts unusable in current develop).Instead of just removing the duplicated declaration I decided to remove public variable entirely and go with a getter function.
I also get the data type to
int
for backwards compatibility and in case we eventually decide to do sth more with this rather than a bool check.PS: With this (hopefully) last PR, we can finally restart/do some analysis with AthenaPK in sync with Parthenon
develop
again.PR Checklist