Allow overriding default allocator in deepCopyToConduit #1746
Conversation
gunney1
requested review from
Arlie-Capps,
cyrush,
kennyweiss and
rhornung67
gunney1
requested review from
BradWhitlock,
bmhan12,
nselliott and
publixsubfan
BradWhitlock
left a comment
BradWhitlock
left a comment
There was a problem hiding this comment.
Looks fine to me. Thanks.
kennyweiss
left a comment
kennyweiss
left a comment
There was a problem hiding this comment.
Thanks @gunney1.
I had one concern about checking that the data was actually copied. I'm not sure if this should hold up the PR, but having this check might help diagnose problems later (e.g. by ruling out some simple issues).
| TEST(sidre_view, deep_copy_to_conduit_with_allocid) | ||
| { | ||
| using axom::sidre::ConduitMemory; | ||
|
|
||
| std::vector<int> allocIds = getKnownAllocIds(); | ||
| for(axom::IndexType si = 0; si < allocIds.size(); ++si) | ||
| { | ||
| auto aId = allocIds[si]; | ||
| auto cId = ConduitMemory::axomAllocIdToConduit(aId); | ||
| std::cout << "si=" << si << " axomAllocId=" << aId << " conduitAllocId=" << cId << std::endl; | ||
| } | ||
| if(allocIds.size() < 2) | ||
| { | ||
| std::cout << "Skipping test deep_copy_to_conduit_with_allocid." | ||
| "At least 2 allocators are needed to test." | ||
| << std::endl; | ||
| return; | ||
| } | ||
|
|
||
| DataStore ds; | ||
|
|
||
| Group* srcGrp = ds.getRoot()->createGroup("src"); | ||
| View* srcView = srcGrp->createView("a"); | ||
| srcView->setScalar(int(15)); | ||
|
|
||
| // Copy srcView to conduit destinations, varying the allocator. | ||
| for(axom::IndexType si = 0; si < allocIds.size(); ++si) | ||
| { | ||
| // dst1 uses default Conduit allocator | ||
| conduit::Node dst1; | ||
| auto defaultConduitAllocId = dst1.allocator(); | ||
| srcView->deepCopyToConduit(dst1); | ||
| EXPECT_EQ(dst1.allocator(), defaultConduitAllocId); | ||
|
|
||
| // dst2 sets and uses allocator allocIds[si] | ||
| conduit::Node dst2; | ||
| dst2.set_allocator(ConduitMemory::axomAllocIdToConduit(allocIds[si])); | ||
| srcView->deepCopyToConduit(dst2); | ||
| EXPECT_EQ(ConduitMemory::conduitAllocIdToAxom(dst2.allocator()), allocIds[si]); | ||
|
|
||
| // dst3 sets allocator allocIds[si] but overrides it in the copy | ||
| conduit::Node dst3; | ||
| dst3.set_allocator(ConduitMemory::axomAllocIdToConduit(allocIds[si])); | ||
| srcView->deepCopyToConduit(dst3, allocIds[0]); | ||
| EXPECT_EQ(ConduitMemory::conduitAllocIdToAxom(dst3.allocator()), allocIds[0]); | ||
| } | ||
|
|
||
| ds.getRoot()->destroyGroup("src"); | ||
| } |
There was a problem hiding this comment.
This new test is checking that we can successfully call deepCopyToConduit in several ways, but it does not verify that the data was actually copied. Is there a way we can check the data?
One idea might be to copy using the data using the specified allocator and then to copy it back to a new buffer using the host allocator and then to check that the data is the same.
Another might be to to do some sort of checksum on the data, and then check that the checksums are the same after copying.
There was a problem hiding this comment.
We have other tests that verify the copied data.
Summary
sidre::Viewto aconduit::Node.This PR takes a bit out of #1736 to make that one smaller, as requested by the code review.