Deep copy from Sidre to Conduit #1510
Open
Conversation
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
The actual fix is on another branch, but I can't wait for it to be merged.
It's failing. Maybe because non-Umpire allocators are not working right or Group isn't using it right.
Also disable test copying in/out of Umpire. This check needs to wait for proper support for that kind of copy, which is being performed on another branch.
change transfer_allocator method name to transferTo. Also use call-back to return View-specific allocator.
gunney1
force-pushed
the
feature/gunney/copy-sidre-to-conduit
branch
from
6764c24 to
41c15d7
Compare
gunney1
force-pushed
the
feature/gunney/copy-sidre-to-conduit
branch
from
582bbad to
46bb8a7
Compare
BradWhitlock
reviewed
Apr 2, 2025
| } | ||
| @endcode | ||
| */ | ||
| struct ConduitMemory |
There was a problem hiding this comment.
It looks like is does the same job as ConduitAllocateThroughAxom.
There was a problem hiding this comment.
It's a similar idea but it directly supports arbitrary Axom (or Umpire) allocator ids, including those of applications and third-party libraries.
gunney1
changed the title
gunney1
requested review from
cyrush,
BradWhitlock,
kennyweiss,
rhornung67,
publixsubfan,
ltaylor16,
bmhan12 and
Arlie-Capps
nselliott
reviewed
Apr 23, 2025
| * deep-copied. | ||
| * | ||
| * The destination's allocator id should be preserved and used for | ||
| * array allocations. However, I'm still checking on this. |
There was a problem hiding this comment.
This work-in-progress comment should be resolved prior to merge.
nselliott
reviewed
Apr 23, 2025
|
|
||
| // TODO: Probably wrong to use owning group's default allocator. | ||
| // Group can allocate View with non-default allocator. | ||
| // int currentAllocId = getOwningGroup()->getDefaultAllocatorID(); |
There was a problem hiding this comment.
It looks like this commented-out section has been resolved and the TODO can be removed.
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.
Summary
Group::deepCopyToConduitandView::deepCopyToConduitViewdata on host. They now allocate in the default allocator of theGroupthat owns theView. (We have discussed this in the project meetings, reaching the concensus was that all data should use the default allocator unless individually overridden, despite the fact that many parts of a hierarchy would only be used on the host.)Views to allocate data on the host, where they are used and expected to be.The goal of this change is toward supporting transparent switching between sidre and conduit. With the addition of
Group::deepCopyToConduit, the state of conduit-sidre hierarchy copies is:* I labeled
importConduitTreeExternalas the shallow conduit-to-sidre copy, but it's not exactly analogous to createNativeLayout (the shallow sidre-to-conduit copy).