Enhancements for ArrayOf[Arrays|Sets] construction/assimilation #256
Conversation
…method ArrayOfArrays::resizeFromOffsets().
| // const_cast needed until for RAJA bug. | ||
| RAJA::inclusive_scan< POLICY >( const_cast< INDEX_TYPE * >( capacities ), | ||
| const_cast< INDEX_TYPE * >( capacities + numSubArrays ), |
There was a problem hiding this comment.
Casts seem no longer needed with current version of RAJA we're using
src/ArrayOfArraysView.hpp
Outdated
| template< typename ... BUFFERS > | ||
| void resizeFromOffsets( INDEX_TYPE const numSubArrays, | ||
| INDEX_TYPE const * const offsets, | ||
| BUFFERS & ... buffers ) |
There was a problem hiding this comment.
I still need to add a unit test for it. But I'd like to get the idea approved first. Usage is as follows:
newAofA.resizeFromOffsets( otherAofA.size(), otherAofA.toView().getOffsets() );We could also implement duplicateSparsity instead, taking a whole AofA (view) as input. Let me know your preferences. It's not clear to me though, whether that method would need to also copy sizes and default construct values. I wouldn't want that to be the case, the new object is meant to be filled via emplaceBacks, not overwriting.
There was a problem hiding this comment.
Yeah now that I think about it (in the context of a CRSMatrix at least) duplicateSparsity would have a different meaning. You'd basically copy all the data wholesale, except set the new values to zero. resizeFromOffsets seems like the correct name, although I'm not a huge fan of passing the pointer and size around, but it's not a big deal.
| this->m_offsets, | ||
| this->m_sizes, | ||
| this->m_values ); | ||
| BUFFER_TYPE< INDEX_TYPE > const sizes = this->m_sizes; |
There was a problem hiding this comment.
There seems to be no way to manually "resize" a single sub-array through view after value deduplication. So I just capture sizes separately and overwrite them, seems to work well.
| // This is not a View test, but it needs a POLICY type argument, so it's placed here for convenience | ||
| void assimilate( IndexType const maxValue, IndexType const maxInserts, sortedArrayManipulation::Description const desc ) |
There was a problem hiding this comment.
Lmk if you'd like a separate test sub-class to contain these assimilation tests (I moved them from ArrayOfSetsTest to ArrayOfSetsViewTest for reason stated in the code comment).
There was a problem hiding this comment.
Nope that sounds just fine. I've done similar things elsewhere.
src/ArrayOfArraysView.hpp
Outdated
| destroyValues( 0, m_numArrays, buffers ... ); | ||
|
|
||
| bufferManipulation::reserve( m_sizes, m_numArrays, MemorySpace::host, numSubArrays ); | ||
| std::fill_n( m_sizes.data(), numSubArrays, 0 ); |
There was a problem hiding this comment.
I know this is just copied from above but maybe this should use umpireInterface::memset. I don't think it's important at the moment but if eventually we want to be able to do this without allocating anything on host it'll be necessary.
| // const_cast needed until for RAJA bug. | ||
| RAJA::inclusive_scan< POLICY >( const_cast< INDEX_TYPE * >( capacities ), | ||
| const_cast< INDEX_TYPE * >( capacities + numSubArrays ), | ||
| RAJA::inclusive_scan< POLICY >( capacities, |
There was a problem hiding this comment.
This function should probably call resizeFromOffsets once the scan has been performed I think.
There was a problem hiding this comment.
So I couldn't just call it, because the result of the scan is written straight into m_offsets, but its old values are still needed in resizeFromOffsets, plus it would then perform a self-copy of m_offsets. I ended up pulling all the shared code into a private function resizeFromOffsetsImpl, which both resizeFromOffsets and resizeFromCapacities delegate to, passing a callback that is invoked when new offsets must be populated.
| this->m_offsets, | ||
| this->m_sizes, | ||
| this->m_values ); | ||
| BUFFER_TYPE< INDEX_TYPE > const sizes = this->m_sizes; |
| // This is not a View test, but it needs a POLICY type argument, so it's placed here for convenience | ||
| void assimilate( IndexType const maxValue, IndexType const maxInserts, sortedArrayManipulation::Description const desc ) |
There was a problem hiding this comment.
Nope that sounds just fine. I've done similar things elsewhere.
klevzoff
force-pushed
the
feature/klevzoff/arrayofsets-assimilate-policy
branch
from
88ac3c1 to
5103a6f
Compare
Related to: GEOS-DEV/GEOS#1847
ArrayOfSets::assimilate()(can be host or device policy).ArrayOfArrays::resizeFromOffsets(). This is useful when a newArrayOfArraysneeds to "inherit" capacity allocations from another existing one (can be extracted viaArrayOfArraysView::getOffsets()), but otherwise be resized to empty state.