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.

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

fix: Restart files contain incorrect types for ArrayOfArrays #3434

Merged
merged 24 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
648818d
add conduit write/read for ArrayOfArrays
rrsettgast Nov 9, 2024
60dd774
Merge branch 'develop' into bugfix/conduitTypeError
rrsettgast Nov 9, 2024
c2b7371
uncrustify
rrsettgast Nov 9, 2024
327b5b9
Merge branch 'bugfix/conduitTypeError' of github.com:GEOS-DEV/GEOS in…
rrsettgast Nov 9, 2024
451572d
update baselines
rrsettgast Nov 9, 2024
362cfcc
fixed bug in ArrayOfArrays conduit reader
rrsettgast Nov 9, 2024
295303b
Merge branch 'develop' into bugfix/conduitTypeError
paveltomin Nov 9, 2024
0451312
add checks to conduit ArrayOfArrays read
rrsettgast Nov 9, 2024
286318f
Merge branch 'bugfix/conduitTypeError' of github.com:GEOS-DEV/GEOS in…
rrsettgast Nov 9, 2024
20b324b
Update .integrated_tests.yaml
paveltomin Nov 10, 2024
28bfe0d
avoid bounds check error using const_cast for ArrayOfArrays:m_values …
rrsettgast Nov 10, 2024
a4569cd
Merge branch 'develop' into bugfix/conduitTypeError
paveltomin Nov 10, 2024
80713cc
more bug-chasing
rrsettgast Nov 10, 2024
cd6ac76
retry ArrayOfArrays::m_values prealloc
rrsettgast Nov 10, 2024
d5305ed
Merge branch 'develop' into bugfix/conduitTypeError
paveltomin Nov 11, 2024
1c00ae8
update baseline hash
rrsettgast Nov 11, 2024
b986147
Merge branch 'bugfix/conduitTypeError' of github.com:GEOS-DEV/GEOS in…
rrsettgast Nov 11, 2024
56eed8f
reset baseline to develop
rrsettgast Nov 11, 2024
76e9594
update baselines....again
rrsettgast Nov 11, 2024
d2e8f8d
make copy of ArrayOfArray::m_values and alter uninitialized allocations
rrsettgast Nov 11, 2024
ac211ad
update baselines
rrsettgast Nov 11, 2024
a799d6b
remove the copy of values
rrsettgast Nov 12, 2024
1be8ad2
uncrustify
rrsettgast Nov 12, 2024
fcfa719
update baseline
rrsettgast Nov 12, 2024
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
2 changes: 1 addition & 1 deletion .integrated_tests.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
baselines:
bucket: geosx
baseline: integratedTests/baseline_integratedTests-pr3372-8610-4b9a7b3
baseline: integratedTests/baseline_integratedTests-pr3434-8609-327b5b9

allow_fail:
all: ''
Expand Down
4 changes: 4 additions & 0 deletions BASELINE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ This file is designed to track changes to the integrated test baselines.
Any developer who updates the baseline ID in the .integrated_tests.yaml file is expected to create an entry in this file with the pull request number, date, and their justification for rebaselining.
These notes should be in reverse-chronological order, and use the following time format: (YYYY-MM-DD).

PR #3434 (2024-11-09)
=====================
Bugfix: Fixed output of ArrayOfArray objects to restart files.

PR #3372 (2024-11-09)
====================
Fix a bug related to mass and energy updates for poromechanics.
Expand Down
106 changes: 106 additions & 0 deletions src/coreComponents/dataRepository/wrapperHelpers.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,112 @@
std::memcpy( var.data(), valuesNode.data_ptr(), numBytesFromArray );
}



template< typename T, typename INDEX_TYPE >
std::enable_if_t< bufferOps::can_memcpy< T > >
pushDataToConduitNode( ArrayOfArrays< T, INDEX_TYPE > const & var2,

Check warning on line 497 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L497

Added line #L497 was not covered by tests
conduit::Node & node )
{
ArrayOfArraysView< T const, INDEX_TYPE > const & var = var2.toViewConst();
internal::logOutputType( LvArray::system::demangleType( var ), "Output array via external pointer: " );

Check warning on line 501 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L500-L501

Added lines #L500 - L501 were not covered by tests

// 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) ) );

Check warning on line 506 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L504-L506

Added lines #L504 - L506 were not covered by tests

// 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 ) ) );

Check warning on line 511 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L509-L511

Added lines #L509 - L511 were not covered by tests

// 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 ) ) );

Check warning on line 516 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L514-L516

Added lines #L514 - L516 were not covered by tests

// Push the data into conduit
constexpr int conduitTypeID = conduitTypeInfo< T >::id;
constexpr int sizeofConduitType = conduitTypeInfo< T >::sizeOfConduitType;
conduit::DataType const dtype( conduitTypeID, offsets[numArrays] * sizeof( T ) / sizeofConduitType );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't fully understand the size here. What' s in offsets[numArrays]?

Shouldn't this be the size of each array * numOfArrays * sizeof(T) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

offsets is the location in the buffer that a subarray starts. The next offset is where the next one starts. So the last value offsets[numArrays] is the end of the buffers as there are numArrays+1 values in offsets. So `offsets[numArrays] is the length of the buffer if I am not mistaken. @corbett5 @wrtobin ??

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I see. I would write this exact comment in the code.

Copy link
Member Author

@rrsettgast rrsettgast Nov 9, 2024

Choose a reason for hiding this comment

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

From ArrayOfArraysView.hpp:

  /// 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;

Copy link
Contributor

@corbett5 corbett5 Nov 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@corbett5 I did notice it would be nice for a bit different nomenclature for this object....for example:

  • there is no accessor function that returns the actual size of the data buffer (offset[m_numArrays])
  • Perhaps language of calling the "arrays" is confusing. I think "subArrays" would be better...however if we want to do an generic NestedArrays arrays object for an arbitrary nesting, it would be nice to come up with something extendible.
  • The accessors for m_offset and m_sizes protected in ArrayOfArraysView. So that means that ArrayOfArrays can't provide public access. I am sure this is by design, but there needs to be more data helper functions to avoid needing direct access to these internal data arrays IMO.

Copy link
Contributor

@corbett5 corbett5 Nov 10, 2024

Choose a reason for hiding this comment

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

1

If you're accessing the underlying data (ie through m_offsets and m_sizes) you could be accessing uninitialized data. Which isn't a problem if you have an ArrayOfArrays< double >, but is very much a problem (or at least you need to know what you're doing) if you have an ArrayOfArrays< string >. So yeah I didn't make this easy on purpose.

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

2

Certainly open for a name change. I thought I tried to call them inner arrays in the docs, but it's certainly confusing.

3

I agree it's strange that you can getValues() becomes accessible once you convert an ArrayOfArrays to an ArrayOfArraysView. I don't think there would be any harm in making these methods accessible directly from ArrayOfArrays.

void * const ptr = const_cast< void * >( static_cast< void const * >( var.getValues() ) );
node[ "__values__" ].set_external( dtype, ptr );
}

Check warning on line 524 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L519-L524

Added lines #L519 - L524 were not covered by tests

template< typename T, typename INDEX_TYPE >
std::enable_if_t< bufferOps::can_memcpy< T > >
pullDataFromConduitNode( ArrayOfArrays< T, INDEX_TYPE > & var,

Check warning on line 528 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L528

Added line #L528 was not covered by tests
conduit::Node const & node )
{

// numArrays node
conduit::Node const & numArraysNode = node.fetch_existing( "__numberOfArrays__" );
INDEX_TYPE const * const numArrays = numArraysNode.value();

Check warning on line 534 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L533-L534

Added lines #L533 - L534 were not covered by tests

// 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();

Check warning on line 540 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L537-L540

Added lines #L537 - L540 were not covered by tests

// 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 warning on line 546 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L543-L546

Added lines #L543 - L546 were not covered by tests

// Check that the numArrays, sizes and offsets are consistent.
GEOS_ERROR_IF_NE( *numArrays, sizeSizes );
GEOS_ERROR_IF_NE( *numArrays+1, sizeOffsets );

Check warning on line 550 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L549-L550

Added lines #L549 - L550 were not covered by tests

// values node
conduit::Node const & valuesNode = node.fetch_existing( "__values__" );
conduit::DataType const & valuesDataType = valuesNode.dtype();
const INDEX_TYPE valuesSize = valuesDataType.number_of_elements();

Check warning on line 555 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L553-L555

Added lines #L553 - L555 were not covered by tests

// resize var with estimated sizes
var.resize( *numArrays, valuesSize/(*numArrays) );

Check warning on line 558 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L558

Added line #L558 was not covered by tests

// correctly set the sizes and capacities of each sub-array
localIndex allocatedSize = 0;
for( INDEX_TYPE i = 0; i < *numArrays; ++i )

Check warning on line 562 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L561-L562

Added lines #L561 - L562 were not covered by tests
{
INDEX_TYPE const arrayAllocation = offsets[i+1] - offsets[i];
var.setCapacityOfArray( i, arrayAllocation );
var.resizeArray( i, sizes[ i ] );
allocatedSize += arrayAllocation;

Check warning on line 567 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L564-L567

Added lines #L564 - L567 were not covered by tests
}

// make sure that the allocated size is the same as the number of values read
GEOS_ERROR_IF_NE( valuesSize, allocatedSize );

Check warning on line 571 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L571

Added line #L571 was not covered by tests

// make sure the allocatedSize is consistent wit the last offset
GEOS_ERROR_IF_NE( allocatedSize, offsets[sizeOffsets-1] );

Check warning on line 574 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L574

Added line #L574 was not covered by tests

// 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 warning on line 579 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L577-L579

Added lines #L577 - L579 were not covered by tests

// 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 warning on line 582 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L582

Added line #L582 was not covered by tests

// check each subarray has the identical capacity and size
for( INDEX_TYPE i = 0; i<*numArrays; ++i )

Check warning on line 585 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L585

Added line #L585 was not covered by tests
{
GEOS_ERROR_IF_NE( varOffsets[i+1], offsets[i+1] );
GEOS_ERROR_IF_NE( varSizes[i], sizes[i] );

Check warning on line 588 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L587-L588

Added lines #L587 - L588 were not covered by tests
}

// copy the values
localIndex numBytesFromArray = allocatedSize * sizeof( T );
GEOS_ERROR_IF_NE( numBytesFromArray, valuesDataType.strided_bytes() );
std::memcpy( &var( 0, 0 ), valuesNode.data_ptr(), numBytesFromArray );
}

Check warning on line 595 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L592-L595

Added lines #L592 - L595 were not covered by tests



template< typename T >
void pushDataToConduitNode( InterObjectRelation< T > const & var,
conduit::Node & node )
Expand Down
Loading
Loading