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 5 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-pr3426-8601-e33a77f
baseline: integratedTests/baseline_integratedTests-pr3434-8609-327b5b9

allow_fail:
all: ''
Expand Down
5 changes: 5 additions & 0 deletions BASELINE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ 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 #3426 (2024-11-08)
=====================
Bugfix: reset accumulation in fracture when time step cut occurs in hydrofrac solver.

PR #3413 (2024-11-07)
Expand Down
94 changes: 94 additions & 0 deletions src/coreComponents/dataRepository/wrapperHelpers.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,100 @@
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

// ArrayOfArray::m_numArrays
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// ArrayOfArray::m_numArrays
// ArrayOfArray::m_numArrays

Copy link
Collaborator

Choose a reason for hiding this comment

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

or maybe you want it as a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

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

// ArrayOfArray::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

// ArrayOfArray::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

// This is an LvArray that doesn't need to be packed.
template< typename T, typename INDEX_TYPE >
std::enable_if_t< bufferOps::can_memcpy< T > >
pullDataFromConduitNode( ArrayOfArrays< T, INDEX_TYPE > & var,

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

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L529

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


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

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L534 - L535 were not covered by tests

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

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 545 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L542-L545

Added lines #L542 - L545 were not covered by tests

GEOS_ERROR_IF_NE( *numArrays, sizeSizes );
GEOS_ERROR_IF_NE( *numArrays+1, sizeOffsets );

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

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L547-L548

Added lines #L547 - L548 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 553 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L551 - L553 were not covered by tests

var.resize( *numArrays, valuesSize/(*numArrays) );
localIndex allocatedSize = 0;
for( INDEX_TYPE i = 0; i < *numArrays; ++i )

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L555 - L557 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 562 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L559 - L562 were not covered by tests
}
ArrayOfArraysView< T const, INDEX_TYPE > const & varView = var.toViewConst();

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

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L564

Added line #L564 was not covered by tests

GEOS_ERROR_IF_NE( valuesSize, allocatedSize );
GEOS_ERROR_IF_NE( allocatedSize, offsets[sizeOffsets] );

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#L566-L567

Added lines #L566 - L567 were not covered by tests

INDEX_TYPE const * const varOffsets = varView.getOffsets();
INDEX_TYPE const * const varSizes = varView.getSizes();

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

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L569-L570

Added lines #L569 - L570 were not covered by tests

GEOS_ERROR_IF_NE( varOffsets[0], offsets[0] );
for( INDEX_TYPE i = 0; i<*numArrays; ++i )

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

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L572-L573

Added lines #L572 - L573 were 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 576 in src/coreComponents/dataRepository/wrapperHelpers.hpp

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L575-L576

Added lines #L575 - L576 were not covered by tests
}


localIndex numBytesFromArray = allocatedSize * sizeof( T );

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

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L580

Added line #L580 was not covered by tests
// GEOS_ERROR_IF_NE( numBytesFromArray, valuesNode.dtype().strided_bytes() );
std::memcpy( &var( 0, 0 ), valuesNode.data_ptr(), numBytesFromArray );
}

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

View check run for this annotation

Codecov / codecov/patch

src/coreComponents/dataRepository/wrapperHelpers.hpp#L582-L583

Added lines #L582 - L583 were not covered by tests



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