Demand struct sync even in CPU mode - #829
Open
TysonRayJones wants to merge 1 commit into
Open
TysonRayJones wants to merge 1 commit into
TysonRayJones wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes synchronisation of heap structs unconditionally necessary, whereas we presently relax this in CPU mode for user convenience - a convenience which makes the API inconsistent across modes and encourages a bug! I have labelled this as an "API break" but it's really "removing a dangerous user shortcut".
Context
Heap structs, like
CompMatr, are dangerous for users to modify/initialise directly because they have a corresponding GPU buffer in GPU mode which must be overwritten. Users are encouraged to use functions likesetCompMatr()which will mark the struct as having been "synced". Passing an unsynced GPU struct to an API function will trigger an error:If a user does not call the setters, and instead opts to manually overwrite the struct elements like above, and if the struct has a GPU buffer, then the user is required to call
syncCompMatr()before passing the struct to functions likeapplyCompMatr(). ThesyncCompMatr()function copies the struct's CPU data to the GPU buffer. If the struct has no GPU buffer (for example, because the QuEST environment is not GPU-accelerated), then the function has no effect, except to mark the struct as "synced".Problem
Presently, we disable sync validation in CPU mode, since it's functionally unnecessary. A user can run the above code without error. But this...
Solution
The solution in this PR is to unconditionally demand heap-structs to be synced - to simply remove the CPU validation relaxation. The error messages are updated accordingly, and some missing sync validation unit tests are added.
The above example becomes:
and runs without error in all modes.
This PR also adds
FullStateDiagMatrsync validation which was erroneously missing fromcalcExpecNonHermitianFullStateDiagMatrandcalcExpecNonHermitianFullStateDiagMatrPower. This bug probably results from these functions not validating Hermiticity, which itself contains the sync check for brevity.