From db1d531df6f9b246143170fd9845106bdb89da83 Mon Sep 17 00:00:00 2001
From: abn <adrien.bruneton@cea.fr>
Date: Thu, 15 Dec 2022 21:44:03 +0100
Subject: [PATCH] Bug fix: MEDFileUMesh::Aggregate wrongly handling duplicated
 families

---
 src/MEDLoader/MEDFileMesh.cxx        | 46 ++++++++++++++++++----------
 src/MEDLoader/Swig/MEDLoaderTest3.py | 40 +++++++++++++-----------
 2 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/src/MEDLoader/MEDFileMesh.cxx b/src/MEDLoader/MEDFileMesh.cxx
index 425c183d0..8bed86939 100644
--- a/src/MEDLoader/MEDFileMesh.cxx
+++ b/src/MEDLoader/MEDFileMesh.cxx
@@ -4882,29 +4882,40 @@ MCAuto<MEDFileUMesh> MEDFileUMesh::Aggregate(const std::vector<const MEDFileUMes
         {
           const std::string& famName = it3.first;
           mcIdType famNum = it3.second;
-          if (famNumMap_rev.find(famNum) != famNumMap_rev.end()) // Family number is already used!
+          if (famNumMap_rev.count(famNum) || famNumMap.count(famName)) // Family number, or family name is already used!
             {
-              // Is it used by a group of the current mesh or a group from a previous mesh?
-              // If not, this is OK (typically -1 familly).
-              bool used = false;
-              //    Current mesh
-              const std::map<std::string, std::vector<std::string> >& locMap2(msh->getGroupInfo());
-              for(const auto& it4 : locMap2)
+              bool needsRenum = true;
+              // Algorithm is as follows: if couple (name, id) do **not** match with the previously registered name and id,
+              // then current family is declared as needing renumbering (needsRenum=True) to avoid strange situations.
+              // Otherwise we just trigger a renum if the family is used by a group (either in the current mesh, or in a previous one).
+              // Thus, if not used by any group at all (but consistent!!), this is actually OK, we can just merge this family by aggregating (typically -1 familly).
+
+              //    1. ID and name matching ?
+              if (famNumMap_rev.count(famNum) && famNumMap_rev.at(famNum) == famName &&
+                  famNumMap.count(famName) && famNumMap.at(famName) == famNum)
+                needsRenum = false;
+
+              //    2. Current mesh
+              if (!needsRenum)
                 {
-                  const auto& famLst = it4.second;
-                  if (std::find(famLst.begin(), famLst.end(), famName) != famLst.end())
-                    { used = true; break; }
+                  const std::map<std::string, std::vector<std::string> >& locMap2(msh->getGroupInfo());
+                  for(const auto& it4 : locMap2)
+                    {
+                      const auto& famLst = it4.second;
+                      if (std::find(famLst.begin(), famLst.end(), famName) != famLst.end())
+                        { needsRenum = true; break; }
+                    }
                 }
-              //    Previous meshes ...
-              if (!used)
+              //    3. Previous meshes ...
+              if (!needsRenum)
                 for(const auto& it4 : grpFamMap)
                   {
                     const auto& famLst = it4.second;
                     if (std::find(famLst.begin(), famLst.end(), famName) != famLst.end())
-                      { used = true; break; }
+                      { needsRenum = true; break; }
                   }
 
-              if(used)
+              if(needsRenum)
                 { // Generate a new family name, and a new family number
                   fam_conflict = true;
                   std::ostringstream oss;
@@ -4916,8 +4927,11 @@ MCAuto<MEDFileUMesh> MEDFileUMesh::Aggregate(const std::vector<const MEDFileUMes
                   famNumMap_rev[min_fam_num] = new_name;
                 }
             }
-          famNumMap[famName] = famNum;
-          famNumMap_rev[famNum] = famName;
+          else
+            {
+              famNumMap[famName] = famNum;
+              famNumMap_rev[famNum] = famName;
+            }
         }
 
       for(const auto& level : levs)
diff --git a/src/MEDLoader/Swig/MEDLoaderTest3.py b/src/MEDLoader/Swig/MEDLoaderTest3.py
index 12842f733..6ff02b41a 100644
--- a/src/MEDLoader/Swig/MEDLoaderTest3.py
+++ b/src/MEDLoader/Swig/MEDLoaderTest3.py
@@ -6184,35 +6184,39 @@ class MEDLoaderTest3(unittest.TestCase):
     @WriteInTmpDir
     def testAggregateWithGroups(self):
         """ Testing MEDFileUMesh::Aggretate when groups are present. """
-        def generate(grp_name, offset):
-            coo = DataArrayDouble([0., 1., 2.])
-            coo += offset
+        def generate(fam, fam_dict, grps, y_offset):
+            """ Generate a 2x3 cartesian grid, with family IDs fam, family names given by the dict,
+                and group given by grps
+            """
             m = MEDCouplingCMesh("toto")
-            m.setCoords(coo, coo)
+            coo_x = DataArrayDouble([0., 1., 2.])
+            coo_y = DataArrayDouble([0., 1., 2., 3.])
+            coo_y += y_offset
+            m.setCoords(coo_x, coo_y)
             m = m.buildUnstructured()
             mu = MEDFileUMesh.New()
             mu.setMeshAtLevel(0, m)
-            g = DataArrayInt([0])
-            g.setName(grp_name)
-            g2 = DataArrayInt([1])
-            g2.setName("common")  # make a common group for all meshes being merged
-            mu.setGroupsAtLevel(0, [g, g2])
+            for f_nam, i in fam_dict.items():
+                mu.setFamilyId(f_nam, i)
+            mu.setFamilyFieldArr(0, DataArrayInt(fam))
+            for g_nam, f_ids in grps.items():
+                mu.setFamiliesIdsOnGroup(g_nam, f_ids)
             return mu
 
-        m1 = generate("A", 0.)
-        m2 = generate("B", 2.)
+        # Two meshes where either family names or family IDs will be conflicting:
+        m1 = generate([-4,-4,-3,-1,-1,-2], {"Fam_-1": -1, "Fam_-2": -2, "Fam_-3": -3, "Fam_-4": -4 }, {"G1": [-1,-2], "G2": [-2]} ,0)
+        m2 = generate([-4,-4,-4,-1,-1,-3], {"Woops_-1": -1, "Fam_-2": -3, "Fam_-4": -4}, {"G1": [-1,-3], "G2": [-3]} , -3)
+
         mm = MEDFileUMesh.Aggregate([m1,m2])
 
-        self.assertEqual(mm.getFamilyFieldAtLevel(0).getValues(), [-2, -3, -1, -1, -4, -5, -1, -1])
+        self.assertEqual(mm.getFamilyFieldAtLevel(0).getValues(),[-4, -4, -3, -1, -1, -2, -4, -4, -4, -6, -6, -5])
         self.assertEqual(mm.getNumberFieldAtLevel(0), None)
-        refFamIds=[('Family_-1',-1),('Family_-2',-2),('Family_-3',-3), ('Family_-4',-4), ('Family_-5',-5)]
+        refFamIds=[('Fam_-1',-1),('Fam_-2',-2),('Fam_-3',-3), ('Fam_-4',-4), ('Family_-5',-5), ('Family_-6',-6)]
         self.assertEqual(set(mm.getFamiliesNames()),set([elt[0] for elt in refFamIds]))
         self.assertEqual(set([mm.getFamilyId(elt) for elt in mm.getFamiliesNames()]),set([elt[1] for elt in refFamIds]))
-        self.assertEqual(mm.getGroupsNames(),('A','B', 'common'))
-        self.assertEqual(mm.getGroupArr(0, 'A').getValues(), [0])
-        self.assertEqual(mm.getGroupArr(0, 'B').getValues(), [4])
-        self.assertEqual(mm.getGroupArr(0, 'common').getValues(), [1,5])
-
+        self.assertEqual(mm.getGroupsNames(),('G1','G2'))
+        self.assertEqual(mm.getGroupArr(0, 'G1').getValues(), [3,4,5,9,10,11])
+        self.assertEqual(mm.getGroupArr(0, 'G2').getValues(), [5, 11])
         pass
 
     @WriteInTmpDir
-- 
2.39.2