fix: Restart files contain incorrect types for ArrayOfArrays #3434
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -494,107 +494,128 @@ | |
|
|
||
| template< typename T, typename INDEX_TYPE > | ||
| std::enable_if_t< bufferOps::can_memcpy< T > > | ||
| pushDataToConduitNode( ArrayOfArrays< T, INDEX_TYPE > const & var2, | ||
| conduit::Node & node ) | ||
| { | ||
| ArrayOfArraysView< T const, INDEX_TYPE > const & var = var2.toViewConst(); | ||
| internal::logOutputType( LvArray::system::demangleType( var ), "Output array via external pointer: " ); | ||
|
|
||
| // ArrayOfArrays::m_numArrays | ||
| INDEX_TYPE const numArrays = var.size(); | ||
| conduit::DataType const numArraysType( conduitTypeInfo< INDEX_TYPE >::id, 1 ); | ||
| node[ "__numberOfArrays__" ].set( numArraysType, const_cast< void * >( static_cast< void const * >(&numArrays) ) ); | ||
|
|
||
| // ArrayOfArrays::m_offsets | ||
| INDEX_TYPE const * const offsets = var.getOffsets(); | ||
| conduit::DataType const offsetsType( conduitTypeInfo< INDEX_TYPE >::id, numArrays+1 ); | ||
| node[ "__offsets__" ].set_external( offsetsType, const_cast< void * >( static_cast< void const * >( offsets ) ) ); | ||
|
|
||
| // ArrayOfArrays::m_sizes | ||
| INDEX_TYPE const * const sizes = var.getSizes(); | ||
| conduit::DataType const sizesType( conduitTypeInfo< INDEX_TYPE >::id, numArrays ); | ||
| node[ "__sizes__" ].set_external( sizesType, const_cast< void * >( static_cast< void const * >( sizes ) ) ); | ||
|
|
||
| // **** WARNING: alters the uninitialized values in the ArrayOfArrays **** | ||
| T const * const values = var.getValues(); | ||
| std::vector< T > valuesCopy( values, values + offsets[numArrays] ); | ||
| for( INDEX_TYPE i = 0; i < numArrays; ++i ) | ||
| { | ||
| INDEX_TYPE const curOffset = offsets[ i ]; | ||
| INDEX_TYPE const nextOffset = offsets[ i + 1 ]; | ||
| for( INDEX_TYPE j = curOffset + var.sizeOfArray( i ); j < nextOffset; ++j ) | ||
| { | ||
| if constexpr ( std::is_arithmetic< T >::value ) | ||
| { | ||
| valuesCopy[ j ] = 0; | ||
| } | ||
| else | ||
| { | ||
| valuesCopy[ j ] = T(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| constexpr int conduitTypeID = conduitTypeInfo< T >::id; | ||
| constexpr int sizeofConduitType = conduitTypeInfo< T >::sizeOfConduitType; | ||
| conduit::DataType const dtype( conduitTypeID, offsets[numArrays] * sizeof( T ) / sizeofConduitType ); | ||
|
There was a problem hiding this comment. I don't fully understand the size here. What' s in Shouldn't this be the size of each array * There was a problem hiding this comment.
There was a problem hiding this comment. oh I see. I would write this exact comment in the code. There was a problem hiding this comment. From /// The number of arrays contained.
INDEX_TYPE_NC m_numArrays = 0;
/// Holds the offset of each array, of length m_numArrays + 1. Array i begins at
/// m_offsets[ i ] and has capacity m_offsets[i+1] - m_offsets[ i ].
BUFFER_TYPE< INDEX_TYPE > m_offsets;
/// Holds the size of each array.
BUFFER_TYPE< SIZE_TYPE > m_sizes;
/// Holds the values of each array. Values in the range [m_offsets[ i ], m_offsets[ i ] + m_sizes[ i ])
/// are valid. All other entries contain uninitialized values.
BUFFER_TYPE< T > m_values;There was a problem hiding this comment. I agree with Randy. The last offset is one measure of the total length of the underlying memory. Other measures are the capacity from the underlying buffer, or offsets(numArrays - 1) + sizeOfArray(numArrays - 1) which would be the end of the valid values. For io you should only need to write up to this last value. There was a problem hiding this comment. @corbett5 I did notice it would be nice for a bit different nomenclature for this object....for example:
There was a problem hiding this comment. 1If you're accessing the underlying data (ie through There is a way to get the total underlying buffer capacity /**
* @return Return the total number values that can be stored before reallocation.
*/
LVARRAY_HOST_DEVICE constexpr inline
INDEX_TYPE_NC valueCapacity() const
{ return m_values.capacity(); }One way to do IO for the whole buffer would be std::copy(arrayOfArryays[1].begin(), arrayOfArrays[arrayOfArrays.size() - 1].end(), dst_iter);2Certainly open for a name change. I thought I tried to call them inner arrays in the docs, but it's certainly confusing. 3I agree it's strange that you can |
||
|
|
||
| // Push the data into conduit | ||
| void * const ptr = valuesCopy.data(); | ||
| node[ "__values__" ].set_external( dtype, ptr ); | ||
| } | ||
|
|
||
| template< typename T, typename INDEX_TYPE > | ||
| std::enable_if_t< bufferOps::can_memcpy< T > > | ||
| pullDataFromConduitNode( ArrayOfArrays< T, INDEX_TYPE > & var, | ||
| conduit::Node const & node ) | ||
| { | ||
|
|
||
| // numArrays node | ||
| conduit::Node const & numArraysNode = node.fetch_existing( "__numberOfArrays__" ); | ||
| INDEX_TYPE const * const numArrays = numArraysNode.value(); | ||
|
|
||
| // offsets node | ||
| conduit::Node const & offsetsNode = node.fetch_existing( "__offsets__" ); | ||
| conduit::DataType const & offsetsDataType = offsetsNode.dtype(); | ||
| INDEX_TYPE const * const offsets = offsetsNode.value(); | ||
| INDEX_TYPE const sizeOffsets = offsetsDataType.number_of_elements(); | ||
|
|
||
| // sizes node | ||
| conduit::Node const & sizesNode = node.fetch_existing( "__sizes__" ); | ||
| conduit::DataType const & sizesDataType = sizesNode.dtype(); | ||
| INDEX_TYPE const * const sizes = sizesNode.value(); | ||
| INDEX_TYPE const sizeSizes = sizesDataType.number_of_elements(); | ||
|
|
||
| // Check that the numArrays, sizes and offsets are consistent. | ||
| GEOS_ERROR_IF_NE( *numArrays, sizeSizes ); | ||
| GEOS_ERROR_IF_NE( *numArrays+1, sizeOffsets ); | ||
|
|
||
| // values node | ||
| conduit::Node const & valuesNode = node.fetch_existing( "__values__" ); | ||
| conduit::DataType const & valuesDataType = valuesNode.dtype(); | ||
| const INDEX_TYPE valuesSize = valuesDataType.number_of_elements(); | ||
|
|
||
| // should preallocate var.m_values with estimated sizes | ||
| INDEX_TYPE const arraySizeEstimate = (*numArrays)==0 ? 0 : valuesSize / (*numArrays); | ||
| var.resize( *numArrays, arraySizeEstimate ); | ||
| var.reserveValues( valuesSize ); | ||
|
|
||
| // correctly set the sizes and capacities of each sub-array | ||
| localIndex allocatedSize = 0; | ||
| for( INDEX_TYPE i = 0; i < *numArrays; ++i ) | ||
| { | ||
| INDEX_TYPE const arrayAllocation = offsets[i+1] - offsets[i]; | ||
| var.setCapacityOfArray( i, arrayAllocation ); | ||
| var.resizeArray( i, sizes[ i ] ); | ||
| allocatedSize += arrayAllocation; | ||
| } | ||
|
|
||
| // make sure that the allocated size is the same as the number of values read | ||
| GEOS_ERROR_IF_NE( valuesSize, allocatedSize ); | ||
|
|
||
| // make sure the allocatedSize is consistent wit the last offset | ||
| GEOS_ERROR_IF_NE( allocatedSize, offsets[sizeOffsets-1] ); | ||
|
|
||
| // get a view because the ArrayOfArraysView data accessors are protected | ||
| ArrayOfArraysView< T const, INDEX_TYPE > const & varView = var.toViewConst(); | ||
| INDEX_TYPE const * const varOffsets = varView.getOffsets(); | ||
| INDEX_TYPE const * const varSizes = varView.getSizes(); | ||
|
|
||
| // check that the offsets that are read are the same as the ones that were allocated | ||
| GEOS_ERROR_IF_NE( varOffsets[0], offsets[0] ); | ||
|
|
||
| // check each subarray has the identical capacity and size | ||
| for( INDEX_TYPE i = 0; i<*numArrays; ++i ) | ||
| { | ||
| GEOS_ERROR_IF_NE( varOffsets[i+1], offsets[i+1] ); | ||
| GEOS_ERROR_IF_NE( varSizes[i], sizes[i] ); | ||
| } | ||
|
|
||
| // copy the values | ||
| localIndex numBytesFromArray = allocatedSize * sizeof( T ); | ||
| GEOS_ERROR_IF_NE( numBytesFromArray, valuesDataType.strided_bytes() ); | ||
| std::memcpy( const_cast< T * >(varView.getValues()), valuesNode.data_ptr(), numBytesFromArray ); | ||
| } | ||
|
|
||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this redundant since
T() = 0ifstd::is_arithmetic< T >::value == true? Also, why do you choose to make a copy instead of changing the values in the array? If we're going to make a copy anyways I'd suggest compressing out the uninitialized values. The only "problem" with this is that when we read the array back in it is compressed as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T() = 0was actually true. If we have a variable declared asint a;, isaguaranteed to be equal to zero?constpromise on the write function. It is of course a double edge sword, because you are not reading in what was passed to the write function...but then changing theconstobject that was passed to the write function seems somewhat illegal?...nevermind...can't make a local copy. Have to break constness.ArrayOfArrayswould result in behavioral changes as adding new values would require different operations after restart....which of course should be OK...unless there is some sort of allocation error that occurs when you try to insert new values in theArrayOfArrays...etc. This is of course ignoring the fact that the array you read back in will not be exactly identical to the one you wrote out in the "unallocated" values....but those shouldn't matter ever or we have bigger problems.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int a;is different fromint a = int(). The first is uninitialized and can be anything, the second calls the default constructor which for numeric types always returns zero.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corbett5
I should know that. thanks!