Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements for ArrayOf[Arrays|Sets] construction/assimilation #256

Merged
merged 3 commits into from
Apr 3, 2022
Merged

Enhancements for ArrayOf[Arrays|Sets] construction/assimilation #256

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a1b625
Add execution policy parameter to ArrayOfSets::assimilate(). Add new …
klevzoff Mar 30, 2022
3eb7a7b
Add unit test for ArrayOfArrays::resizeFromOffsets.
klevzoff Mar 30, 2022
5103a6f
Extract common code of ArrayOfArraysView::resizeFrom functions
klevzoff Mar 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions examples/exampleArrayOfSets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ TEST( ArrayOfSets, assimilate )
arrayOfArrays.emplaceBack( 2, 1 );

// Assimilate arrayOfArrays into arrayOfSets.
arrayOfSets.assimilate( std::move( arrayOfArrays ),
LvArray::sortedArrayManipulation::Description::SORTED_UNIQUE );
arrayOfSets.assimilate< RAJA::loop_exec >( std::move( arrayOfArrays ),
LvArray::sortedArrayManipulation::Description::SORTED_UNIQUE );

// After being assimilated arrayOfArrays is empty.
EXPECT_EQ( arrayOfArrays.size(), 0 );
Expand All @@ -84,8 +84,8 @@ TEST( ArrayOfSets, assimilate )
arrayOfArrays.emplaceBack( 1, 4 );

// Assimilate the arrayOfArrays yet again.
arrayOfSets.assimilate( std::move( arrayOfArrays ),
LvArray::sortedArrayManipulation::Description::UNSORTED_WITH_DUPLICATES );
arrayOfSets.assimilate< RAJA::loop_exec >( std::move( arrayOfArrays ),
LvArray::sortedArrayManipulation::Description::UNSORTED_WITH_DUPLICATES );

EXPECT_EQ( arrayOfSets.size(), 2 );
EXPECT_EQ( arrayOfSets.sizeOfSet( 0 ), 2 );
Expand Down
1 change: 1 addition & 0 deletions src/ArrayOfArrays.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ class ArrayOfArrays : protected ArrayOfArraysView< T, INDEX_TYPE, false, BUFFER_
}

using ParentClass::resizeFromCapacities;
using ParentClass::resizeFromOffsets;

///@}

Expand Down
40 changes: 37 additions & 3 deletions src/ArrayOfArraysView.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// Source includes
#include "bufferManipulation.hpp"
#include "arrayManipulation.hpp"
#include "sortedArrayManipulation.hpp"
#include "ArraySlice.hpp"
#include "typeManipulation.hpp"
#include "math.hpp"
Expand Down Expand Up @@ -720,9 +721,8 @@ class ArrayOfArraysView
// RAJA::inclusive_scan fails on empty input range
if( numSubArrays > 0 )
{
// const_cast needed until for RAJA bug.
RAJA::inclusive_scan< POLICY >( const_cast< INDEX_TYPE * >( capacities ),
const_cast< INDEX_TYPE * >( capacities + numSubArrays ),
Comment on lines -723 to -725
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casts seem no longer needed with current version of RAJA we're using

RAJA::inclusive_scan< POLICY >( capacities,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should probably call resizeFromOffsets once the scan has been performed I think.

Copy link
Contributor Author

@klevzoff klevzoff Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

capacities + numSubArrays,
m_offsets.data() + 1 );
}

Expand All @@ -734,6 +734,40 @@ class ArrayOfArraysView
}, m_values, buffers ... );
}

/**
* @brief Clears the array and creates a new array with the given number of sub-arrays.
* @param numSubArrays The new number of arrays.
* @param offsets A pointer to an array of length @p numSubArrays+1 containing the offset
* of each new sub array. Offsets are precomputed by the caller.
* @param buffers A variadic pack of buffers to treat similarly to m_values.
*/
template< typename ... BUFFERS >
void resizeFromOffsets( INDEX_TYPE const numSubArrays,
INDEX_TYPE const * const offsets,
BUFFERS & ... buffers )
Copy link
Contributor Author

@klevzoff klevzoff Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
LVARRAY_ASSERT( arrayManipulation::isPositive( numSubArrays ) );
LVARRAY_ASSERT_EQ( offsets[0], 0 );
LVARRAY_ASSERT( sortedArrayManipulation::isSorted( offsets, offsets + numSubArrays + 1 ) );

destroyValues( 0, m_numArrays, buffers ... );

bufferManipulation::reserve( m_sizes, m_numArrays, MemorySpace::host, numSubArrays );
std::fill_n( m_sizes.data(), numSubArrays, 0 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done


INDEX_TYPE const offsetsSize = ( m_numArrays == 0 ) ? 0 : m_numArrays + 1;
bufferManipulation::reserve( m_offsets, offsetsSize, MemorySpace::host, numSubArrays + 1 );

arrayManipulation::uninitializedCopy( offsets, offsets + numSubArrays + 1, m_offsets.data() );

m_numArrays = numSubArrays;
INDEX_TYPE const maxOffset = m_offsets[ m_numArrays ];
typeManipulation::forEachArg( [ maxOffset ] ( auto & buffer )
{
bufferManipulation::reserve( buffer, 0, MemorySpace::host, maxOffset );
}, m_values, buffers ... );
}

///@}

/**
Expand Down
60 changes: 33 additions & 27 deletions src/ArrayOfSets.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,11 @@ class ArrayOfSets : protected ArrayOfSetsView< T, INDEX_TYPE, BUFFER_TYPE >

/**
* @brief Steal the resources from an ArrayOfArrays and convert it to an ArrayOfSets.
* @tparam POLICY a RAJA execution policy to use when sorting/removing duplicates in sub-arrays.
* @param src the ArrayOfArrays to convert.
* @param desc describes the type of data in the source.
* TODO: Add a RAJA policy template parameter.
*/
template< typename POLICY >
inline
void assimilate( ArrayOfArrays< T, INDEX_TYPE, BUFFER_TYPE > && src,
sortedArrayManipulation::Description const desc )
Expand All @@ -133,38 +134,43 @@ class ArrayOfSets : protected ArrayOfSetsView< T, INDEX_TYPE, BUFFER_TYPE >
ParentClass::assimilate( reinterpret_cast< ParentClass && >( src ) );

INDEX_TYPE const numSets = size();
ArrayOfArraysView< T, INDEX_TYPE const, true, BUFFER_TYPE > const view =
ArrayOfArraysView< T, INDEX_TYPE const, true, BUFFER_TYPE >( numSets,
this->m_offsets,
this->m_sizes,
this->m_values );
BUFFER_TYPE< INDEX_TYPE > const sizes = this->m_sizes;
Copy link
Contributor Author

@klevzoff klevzoff Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.


if( desc == sortedArrayManipulation::UNSORTED_NO_DUPLICATES )
{
for( INDEX_TYPE i = 0; i < numSets; ++i )
{
T * const setValues = getSetValues( i );
INDEX_TYPE const numValues = sizeOfSet( i );
std::sort( setValues, setValues + numValues );
}
RAJA::forall< POLICY >( RAJA::TypedRangeSegment< INDEX_TYPE >( 0, numSets ),
[view] LVARRAY_HOST_DEVICE ( INDEX_TYPE const i )
{
ArraySlice< T, 1, 0, INDEX_TYPE > const setValues = view[i];
sortedArrayManipulation::makeSorted( setValues.begin(), setValues.end() );
} );
}
if( desc == sortedArrayManipulation::SORTED_WITH_DUPLICATES )
else if( desc == sortedArrayManipulation::SORTED_WITH_DUPLICATES )
{
for( INDEX_TYPE i = 0; i < numSets; ++i )
{
T * const setValues = getSetValues( i );
INDEX_TYPE const numValues = sizeOfSet( i );

INDEX_TYPE const numUniqueValues = sortedArrayManipulation::removeDuplicates( setValues, setValues + numValues );
arrayManipulation::resize( setValues, numValues, numUniqueValues );
this->m_sizes[ i ] = numUniqueValues;
}
RAJA::forall< POLICY >( RAJA::TypedRangeSegment< INDEX_TYPE >( 0, numSets ),
[view, sizes] LVARRAY_HOST_DEVICE ( INDEX_TYPE const i )
{
ArraySlice< T, 1, 0, INDEX_TYPE > const setValues = view[i];
INDEX_TYPE const numUniqueValues = sortedArrayManipulation::removeDuplicates( setValues.begin(), setValues.end() );
arrayManipulation::resize< T >( setValues, setValues.size(), numUniqueValues );
sizes[ i ] = numUniqueValues;
} );
}
if( desc == sortedArrayManipulation::UNSORTED_WITH_DUPLICATES )
else if( desc == sortedArrayManipulation::UNSORTED_WITH_DUPLICATES )
{
for( INDEX_TYPE i = 0; i < numSets; ++i )
{
T * const setValues = getSetValues( i );
INDEX_TYPE const numValues = sizeOfSet( i );

INDEX_TYPE const numUniqueValues = sortedArrayManipulation::makeSortedUnique( setValues, setValues + numValues );
arrayManipulation::resize( setValues, numValues, numUniqueValues );
this->m_sizes[ i ] = numUniqueValues;
}
RAJA::forall< POLICY >( RAJA::TypedRangeSegment< INDEX_TYPE >( 0, numSets ),
[view, sizes] LVARRAY_HOST_DEVICE ( INDEX_TYPE const i )
{
ArraySlice< T, 1, 0, INDEX_TYPE > const setValues = view[ i ];
INDEX_TYPE const numUniqueValues = sortedArrayManipulation::makeSortedUnique( setValues.begin(), setValues.end() );
arrayManipulation::resize< T >( setValues, setValues.size(), numUniqueValues );
sizes[ i ] = numUniqueValues;
} );
}

#ifdef ARRAY_BOUNDS_CHECK
Expand Down
44 changes: 44 additions & 0 deletions unitTests/testArrayOfArrays.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,41 @@ class ArrayOfArraysTest : public ::testing::Test
COMPARE_TO_REFERENCE;
}

void resizeFromOffsets( IndexType const newSize, IndexType const maxCapacity )
{
COMPARE_TO_REFERENCE;

std::vector< IndexType > newCapacities( newSize );

for( IndexType & capacity : newCapacities )
{
capacity = rand( 0, maxCapacity );
}

std::vector< IndexType > newOffsets( newSize + 1 );

IndexType totalOffset = 0;
for( IndexType i = 0; i < newSize; ++i )
{
newOffsets[i] = totalOffset;
totalOffset += newCapacities[i];
}
newOffsets.back() = totalOffset;

m_array.resizeFromOffsets( newSize, newOffsets.data() );

EXPECT_EQ( m_array.size(), newSize );
for( IndexType i = 0; i < m_array.size(); ++i )
{
EXPECT_EQ( m_array.sizeOfArray( i ), 0 );
EXPECT_EQ( m_array.capacityOfArray( i ), newCapacities[ i ] );
}

m_ref.clear();
m_ref.resize( newSize );
COMPARE_TO_REFERENCE;
}

void resize()
{
COMPARE_TO_REFERENCE;
Expand Down Expand Up @@ -749,6 +784,15 @@ TYPED_TEST( ArrayOfArraysTest, resizeFromCapacities )
}
}

TYPED_TEST( ArrayOfArraysTest, resizeFromOffsets )
{
for( int i = 0; i < 3; ++i )
{
this->resizeFromOffsets( 100, 10 );
this->emplace( 10 );
}
}

TYPED_TEST( ArrayOfArraysTest, resizeArray )
{
this->resize( 100 );
Expand Down
129 changes: 66 additions & 63 deletions unitTests/testArrayOfSets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,45 +429,6 @@ class ArrayOfSetsTest : public ::testing::Test
}
}

void assimilate( IndexType const maxValue, IndexType const maxInserts, sortedArrayManipulation::Description const desc )
{
IndexType const nSets = m_array.size();
ArrayOfArraysT arrayToSteal( nSets );

for( IndexType i = 0; i < nSets; ++i )
{
IndexType const nValues = rand( 0, maxInserts );

for( IndexType j = 0; j < nValues; ++j )
{
T const value = T( rand( 0, maxValue ));
bool const insertSuccess = m_ref[ i ].insert( value ).second;

if( sortedArrayManipulation::isUnique( desc ))
{
if( insertSuccess )
{
arrayToSteal.emplaceBack( i, value );
}
}
else
{
arrayToSteal.emplaceBack( i, value );
}
}

if( sortedArrayManipulation::isSorted( desc ))
{
T * const values = arrayToSteal[ i ];
std::sort( values, values + arrayToSteal.sizeOfArray( i ));
}
}

m_array.assimilate( std::move( arrayToSteal ), desc );

COMPARE_TO_REFERENCE
}

protected:

template< typename CONTAINER >
Expand Down Expand Up @@ -691,30 +652,6 @@ TYPED_TEST( ArrayOfSetsTest, shallowCopy )
this->shallowCopy();
}

TYPED_TEST( ArrayOfSetsTest, assimilateSortedUnique )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::SORTED_UNIQUE );
}

TYPED_TEST( ArrayOfSetsTest, assimilateUnsortedNoDuplicates )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::UNSORTED_NO_DUPLICATES );
}

TYPED_TEST( ArrayOfSetsTest, assimilateSortedWithDuplicates )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::SORTED_WITH_DUPLICATES );
}

TYPED_TEST( ArrayOfSetsTest, assimilateUnsortedWithDuplicates )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::UNSORTED_WITH_DUPLICATES );
}

// This is testing capabilities of the ArrayOfArrays class, however it needs to first populate
// the ArrayOfSets so it involves less code duplication to put it here.
TYPED_TEST( ArrayOfSetsTest, ArrayOfArraysStealFrom )
Expand Down Expand Up @@ -754,6 +691,7 @@ class ArrayOfSetsViewTest : public ArrayOfSetsTest< typename ARRAY_OF_SETS_POLIC
using typename ParentClass::IndexType;
using typename ParentClass::ViewType;
using typename ParentClass::ViewTypeConst;
using typename ParentClass::ArrayOfArraysT;

template< typename U >
using Array1D = typename ToArray< U, ARRAY_OF_SETS >::OneD;
Expand Down Expand Up @@ -890,6 +828,47 @@ class ArrayOfSetsViewTest : public ArrayOfSetsTest< typename ARRAY_OF_SETS_POLIC
COMPARE_TO_REFERENCE
}

// 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 )
Comment on lines +831 to +832
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope that sounds just fine. I've done similar things elsewhere.

{
IndexType const nSets = m_array.size();
ArrayOfArraysT arrayToSteal( nSets );

for( IndexType i = 0; i < nSets; ++i )
{
IndexType const nValues = rand( 0, maxInserts );

for( IndexType j = 0; j < nValues; ++j )
{
T const value = T( rand( 0, maxValue ));
bool const insertSuccess = m_ref[ i ].insert( value ).second;

if( sortedArrayManipulation::isUnique( desc ))
{
if( insertSuccess )
{
arrayToSteal.emplaceBack( i, value );
}
}
else
{
arrayToSteal.emplaceBack( i, value );
}
}

if( sortedArrayManipulation::isSorted( desc ))
{
T * const values = arrayToSteal[ i ];
std::sort( values, values + arrayToSteal.sizeOfArray( i ));
}
}

m_array.template assimilate< POLICY >( std::move( arrayToSteal ), desc );

m_array.move( MemorySpace::host );
COMPARE_TO_REFERENCE
}

protected:

Array1D< Array1D< T > > createValues( bool const insert, bool const sortedUnique )
Expand Down Expand Up @@ -1012,6 +991,30 @@ TYPED_TEST( ArrayOfSetsViewTest, removeMultiple )
}
}

TYPED_TEST( ArrayOfSetsViewTest, assimilateSortedUnique )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::SORTED_UNIQUE );
}

TYPED_TEST( ArrayOfSetsViewTest, assimilateUnsortedNoDuplicates )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::UNSORTED_NO_DUPLICATES );
}

TYPED_TEST( ArrayOfSetsViewTest, assimilateSortedWithDuplicates )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::SORTED_WITH_DUPLICATES );
}

TYPED_TEST( ArrayOfSetsViewTest, assimilateUnsortedWithDuplicates )
{
this->resize( 50 );
this->assimilate( DEFAULT_MAX_INSERTS, DEFAULT_MAX_VALUE, sortedArrayManipulation::UNSORTED_WITH_DUPLICATES );
}

} // namespace testing
} // namespace LvArray

Expand Down