fix: error in MappedVector::insert #3610

rrsettgast · 2025-03-26T18:25:28Z

MappedVector::insert incorrectly delete object if it is a duplicate entry and the overwrite flag isn't true. This means that the insert is a no-op, and the object that was supposed to be inserted was deleted. Bad news bears.

fix incorrect deletion of objects referred to by input pointer
removed and disallowed redundant registrations, but provided a registration option to permit existence.
removed the redundant name parameter from certain registerGroup and registerWrapper methods.

Related to: GEOS-DEV/LvArray#350

rrsettgast · 2025-03-29T00:53:24Z

@wrtobin I made the redundant registration attempts an error if the registration doesn't have the same exact pointer address as the existing entry. This is the only case that is OK imo. There are a lot of errors. Things seem to be registered in the mesh generation/ mesh level creation in a quite inconsistent way. No big surprise. Can you have a go at resolving some the errors?

Copilot

Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.

Files not reviewed (2)

src/coreComponents/schema/schema.xsd: Language not supported
src/coreComponents/schema/schema.xsd.other: Language not supported

Comments suppressed due to low confidence (1)

src/coreComponents/dataRepository/MappedVector.hpp:510

If 'takeOwnership' is true but the source pointer differs from the existing value, the code only updates the ownership flag without replacing the pointer. Verify that this logic aligns with the intended behavior to correctly handle duplicate entries.

    else if( m_values[index].second != source ) // this insertion is OK if the source pointer is the same address as the existing value

src/coreComponents/dataRepository/MappedVector.hpp

CusiniM · 2025-04-11T22:54:32Z

src/coreComponents/mesh/MeshLevel.cpp

      arrayView1d< bool const > const nodeInCurSet = nodeInSet[setName];

-      SortedArray< localIndex > & targetSet = elementSets.registerWrapper< SortedArray< localIndex > >( setName ).reference();
+//      SortedArray< localIndex > & targetSet = elementSets.registerWrapper< SortedArray< localIndex > >( setName ).reference();


Suggested change

// SortedArray< localIndex > & targetSet = elementSets.registerWrapper< SortedArray< localIndex > >( setName ).reference();

src/coreComponents/mesh/MeshLevel.cpp

CusiniM · 2025-04-11T22:55:19Z

src/coreComponents/mesh/MeshManager.cpp

-    MeshBody & meshBody = domain.getMeshBodies().registerGroup< MeshBody >( meshGen.getName() );
-    meshBody.createMeshLevel( 0 );
+    MeshBody & meshBody = domain.getMeshBodies().getGroup< MeshBody >( meshGen.getName() );
+//    meshBody.createMeshLevel( 0 );


CusiniM · 2025-04-11T22:55:51Z

src/coreComponents/mesh/ObjectManagerBase.cpp

    setPlotLevel( PlotLevel::NOPLOT );

-  m_sets.registerWrapper< SortedArray< localIndex > >( this->m_ObjectManagerBaseViewKeys.externalSet );
+//  m_sets.registerWrapper< SortedArray< localIndex > >( this->m_ObjectManagerBaseViewKeys.externalSet );


Suggested change

// m_sets.registerWrapper< SortedArray< localIndex > >( this->m_ObjectManagerBaseViewKeys.externalSet );

corbett5 · 2025-04-14T15:45:32Z

src/coreComponents/dataRepository/MappedVector.hpp

-    }
-
    // if value is empty, then move source into value slot
    if( m_values[index].second==nullptr )


I'm curious, when do we use this functionality of inserting a nullptr? It seems like the kind of thing that could reasonably be disallowed.

....err....good point.

paveltomin · 2025-08-04T17:00:00Z

@rrsettgast is this still active PR?

rrsettgast · 2025-09-16T22:26:26Z

@wrtobin you were supposed to pick this up a while back.

corbett5 · 2025-10-20T17:49:21Z

I'm gonna try and get this working this week.

corbett5 · 2025-11-04T06:56:08Z

inputFiles/lagrangianContactMechanics/TFrac_smoke.xml

 <!-- SPHINX_SOLVER_END -->
 </Solvers>

-  <NumericalMethods>


This is already present in the included base file.

corbett5 · 2025-11-04T06:56:34Z

inputFiles/wavePropagation/acous3D_Q3_abc_fs_smoke.xml

      xMax="{ 101.01, 101.01, 101.01 }"/>
  </Geometry>
-
-    <!-- The free surface condition the domain -->


This is already present in the included base file.

corbett5 · 2025-11-04T06:59:11Z

src/coreComponents/constitutive/ConstitutiveBase.cpp

      {
        string const wrapperName = makeFieldName( this->getName(), wrapper.first );
-        parent.registerWrapper( wrapper.second->clone( wrapperName, parent ) ).
+        parent.registerWrapper( wrapper.second->clone( wrapperName, parent ), true ).


These sorts of changes registerWrapper(..., true) and registerGroup(..., true) permit an object with the given name to already exist, and return the existing object.

corbett5 · 2025-11-04T07:00:02Z

src/coreComponents/constitutive/ConstitutiveManager.cpp

+Group * ConstitutiveManager::createChild( string const & childKey,
+                                          string const & childName,
+                                          bool const allowExistence )


There were some places where createChild needed to change, since this is a virtual function in Group, changing the signature there required changes everywhere.

corbett5 · 2025-11-04T07:02:00Z

src/coreComponents/constitutive/ConstitutiveManager.cpp

  std::unique_ptr< ConstitutiveBase > material =
    ConstitutiveBase::CatalogInterface::factory( childKey, getDataContext(), childName, this );
-  return &registerGroup< ConstitutiveBase >( childName, std::move( material ) );
+  return &registerGroup< ConstitutiveBase >( std::move( material ), allowExistence );


I also made it so that methods such as these, which register an existing Group or Wrapper no longer require an additional name (childName in this example). In every instance in the code the provided name is the same as the name of the Group (or Wrapper itself, and it seems this is something we should enforce.

corbett5 · 2025-11-04T07:21:12Z

src/coreComponents/mesh/ObjectManagerBase.cpp

+  auto & set = m_sets.registerWrapper< SortedArray< localIndex > >( newSetName, true ).reference();
+  set.clear();
+  return set;


this set.clear() behavior is new.

corbett5 · 2025-11-04T07:23:11Z

src/coreComponents/mesh/ObjectManagerBase.hpp

    m_registeredField.insert( FIELD_TRAIT::key());

-    return this->registerWrapper< typename FIELD_TRAIT::type >( FIELD_TRAIT::key() ).
+    return this->registerWrapper< typename FIELD_TRAIT::type >( FIELD_TRAIT::key(), nullptr, true ).


The nullptr here is to satisfy the following signature without having to introduce a similar function without rkey

template< typename T, typename TBASE=T > Wrapper< TBASE > & registerWrapper( string const & name, wrapperMap::KeyIndex::index_type * const rkey = nullptr, bool const allowExistence=false );

corbett5 · 2025-11-04T07:28:38Z

...ysicsSolvers/wavePropagation/sem/elastic/secondOrderEqn/isotropic/ElasticWaveEquationSEM.cpp

                  parent )
 {
-  registerWrapper( viewKeyStruct::sourceConstantsString(), &m_sourceConstantsx ).
+  registerWrapper( viewKeyStruct::sourceConstantsXString(), &m_sourceConstantsx ).


This is one of the true mistakes I found. The few failing integrated tests are due to this and will require a rebaseline.

corbett5 · 2025-11-04T07:30:37Z

src/coreComponents/physicsSolvers/multiphysics/PoromechanicsSolver.hpp

-        subRegion.registerWrapper< string >( viewKeyStruct::porousMaterialNamesString() ).
-          setPlotLevel( dataRepository::PlotLevel::NOPLOT ).
-          setRestartFlags( dataRepository::RestartFlags::NO_WRITE ).
-          setSizedFromParent( 0 );


These must have been registered elsewhere, but I couldn't find where.

corbett5 · 2025-11-04T07:35:41Z

src/coreComponents/mesh/SurfaceElementRegion.cpp

    SortedArrayView< localIndex const > const & faceSet = faceManager->sets().getReference< SortedArray< localIndex > >( setIter.first );
-    SortedArray< localIndex > & faceElementSet = subRegion.sets().registerWrapper< SortedArray< localIndex > >( setIter.first ).reference();
+    SortedArray< localIndex > & faceElementSet = subRegion.sets().getReference< SortedArray< localIndex > >( setIter.first );
+    // faceElementSet.clear();


@rrsettgast For all the set building I initially cleared the set because I mistakenly assumed the old behavior of registerWrapper would replace the previously held data (effectively calling clear). This worked fine everywhere else, in fact I still kept that behavior. But here this presented a problem with some integrated tests having diffs in the the size of FractureElementSubregion/sets/externalSet, but only in parallel.

My question is: do we actually want to clear the sets every time?

corbett5 · 2025-11-06T21:51:14Z

I updated the error messages and conditions as requested. Here are two examples

***** ERROR
***** LOCATION: /usr/WS2/corbett5/GEOS/develop/src/coreComponents/dataRepository/Group.cpp:774
***** Controlling expression (should be false): true
***** Rank 0: Tried registering a wrapper "pressure_n" of type real64_array with "/domain/MeshBodies/mesh/meshLevels/Level0/nodeManager"
A wrapper of this name already exists with type real32_array. The existing wrapper was registered by the following:
        acousticSolver

***** ERROR
***** LOCATION: /usr/WS2/corbett5/GEOS/develop/src/coreComponents/dataRepository/Group.cpp:801
***** Controlling expression (should be false): !allowExistence || (existingTypeId != typeId)
***** Rank 0: Tried registering a Group "acousticSolver" of type geos::AcousticWaveEquationSEM with "/Solvers"
A Group of this name already exists with type geos::dataRepository::Group

rrsettgast requested review from MelReyCG, corbett5 and wrtobin as code owners March 26, 2025 18:25

rrsettgast self-assigned this Mar 27, 2025

rrsettgast requested a review from bd713 March 27, 2025 08:30

rrsettgast requested review from CusiniM and untereiner as code owners March 28, 2025 19:53

rrsettgast assigned wrtobin Mar 29, 2025

wrtobin requested a review from Copilot April 10, 2025 17:27

Copilot AI reviewed Apr 10, 2025

View reviewed changes

src/coreComponents/dataRepository/MappedVector.hpp Outdated Show resolved Hide resolved

CusiniM reviewed Apr 11, 2025

View reviewed changes

src/coreComponents/mesh/MeshLevel.cpp Show resolved Hide resolved

CusiniM reviewed Apr 11, 2025

View reviewed changes

CusiniM approved these changes Apr 11, 2025

View reviewed changes

corbett5 reviewed Apr 14, 2025

View reviewed changes

corbett5 approved these changes Apr 14, 2025

View reviewed changes

corbett5 force-pushed the bugfix/invalidDeleteInMappedVector branch from 574b80c to 617534e Compare October 31, 2025 02:13

corbett5 requested review from OmarDuran, acitrain, castelletto1, dkachuma, herve-gross, jhuang2601, paveltomin, sframba and tjb-ltk as code owners October 31, 2025 02:13

corbett5 requested a review from Guotong-Ren as a code owner November 1, 2025 06:54

rrsettgast and others added 12 commits November 1, 2025 23:23

removed invalid delete in MappedVector

23b6510

remove redundant set registrations

4511ce5

unused var

59fc3d3

Disallow duplicate registration by default.

728060c

Fixed externalSet.

3da4a6e

Fixing more problems with sets.

6f5eadf

Uncrustify changes.

4356899

Documentation.

23fa30a

Fixed some integrated test errors.

95fe965

Doxygen

8133922

Run solver tests with single thread.

6a1804f

Fixing more tests.

653ee13

corbett5 force-pushed the bugfix/invalidDeleteInMappedVector branch from 652a70d to 653ee13 Compare November 2, 2025 06:34

corbett5 added 5 commits November 3, 2025 12:17

Fixed my leak in MappedVector::insert.

12db88d

Removing duplicates from XML files.

fd8f913

Clear set in NodeManager::buildGeometricSets.

a22944d

Not clearing a specific set.

f2e3f87

Reverting some XML changes.

c2b55b9

corbett5 reviewed Nov 4, 2025

View reviewed changes

corbett5 added ci: run CUDA builds Allows to triggers (costly) CUDA jobs flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests type: bug Something isn't working type: cleanup / refactor Non-functional change (NFC) labels Nov 4, 2025

Cleanup

6525094

corbett5 self-requested a review November 4, 2025 07:59

corbett5 mentioned this pull request Nov 6, 2025

Updated clang pragma for designated initializers. GEOS-DEV/LvArray#350

Open

Updated error messages and conditions.

98e3a89

fix: error in MappedVector::insert #3610

Are you sure you want to change the base?

fix: error in MappedVector::insert #3610

Uh oh!

Conversation

rrsettgast commented Mar 26, 2025 • edited by corbett5 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rrsettgast commented Mar 29, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveltomin commented Aug 4, 2025

Uh oh!

rrsettgast commented Sep 16, 2025

Uh oh!

corbett5 commented Oct 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corbett5 commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rrsettgast commented Mar 26, 2025 •

edited by corbett5

Loading