Conversation
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
…' into feature/gunney/copy-sidre-to-conduit
|
Please review this PR as it must be merged soon! |
|
This PR must be completed before #1654. Both are required for the 2025 Fall release. |
kennyweiss
left a comment
kennyweiss
left a comment
There was a problem hiding this comment.
Thanks @gunney1 -- please clean up the primal/klee stuff that leaked in from your other PR
Arlie-Capps
left a comment
Arlie-Capps
left a comment
There was a problem hiding this comment.
@gunney1 , thank you for your hard work on this.
| struct ConduitMemory | ||
| { | ||
| //!@brief Return the Axom allocator id. | ||
| int axomId() const { return m_axomId; } |
There was a problem hiding this comment.
Consistency: Do/Should we use Id or ID or id throughout Axom?
It doesn't look like we've been especially consistent, but we might want to consider this in the future.
See: https://sourcegraph.com/search?q=repo:%5Egithub%5C.com/llnl/axom+Id&patternType=keyword&sm=0
src/axom/sidre/core/View.hpp
Outdated
|
|
||
| // Note: most of these calls that set the view class members are | ||
| // unnecessary if the view already holds a tuple. May be | ||
| // a future optimization opportunity to split the |
There was a problem hiding this comment.
Please finish this sentence.
There was a problem hiding this comment.
I actually don't know how to finish it. It appears multiple times in the file. Aaron wrote the comment and I used his existing methods as a template. I'll remove the fragments.
8 participants
Summary
Group::deepCopyToConduitandView::deepCopyToConduitViews with different allocators based on theViewtype (string, scalar or array). The mechanism generalizes scalars so they can be array-like but treated as scalars for the purpose of data allocation and copying. In use, this can transparently place big computational data on device and metadata on host.bumpcomponent. We will combine the two after this merge.Fix bug that always allocates string and scalarPreserves the unintended but useful behavior of scalars and strings always being allocated on the host. Scalars and strings now have their own allocator, with the option of being placed on device if the user wants.Viewdata 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.)Manually tell a bunch ofViews to allocate data on the host, where they are used and expected to be.The generalized scalar type is called a "tuple", which is treated like a scalar but may have more than one element. This change:
View::setTuple,View::isTuple,Group::setDefaultArrayAllocatorandGroup::setDefaultTupleAllocatorGroup::setDefaultAllocatorandGroup::getDefaultAllocatorto call the array versions of those methodsThe use of tuples greatly simplifies working with sidre hierarchies. Using the same allocator for all data and manually overriding individual leaves, was not workable.
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 tocreateNativeLayout(the shallow sidre-to-conduit copy).