Wrap splits functions - for ParMmg convenience by laetitia-m · Pull Request #225 · MmgTools/mmg

laetitia-m · 2023-10-16T15:23:35Z

Wrap the splits functions used in LS mode (split1, split2sf, split3cone and split4op) for ParMmg convenience. In the splits, the configuration is determined based on the vertices indices. In ParMmg, we need to use the global vertices numbering instead of the local (in term of proc) vertices numbering: to allow this, split functions have been modified to be able to specify the tetra vertices numbering as argument. This change has no impact on mmg.
Rename some split configuration functions for consistency (e.g. MMG3D_configSplit3op becomes MMG3D_split3op_cfg).

… update communicator

…2) Add 2 new functions to get and update k and s variables at the same time in hash table

…e to use global indices of nodes to determine the split configuration)

Algiane

Hi,

I have few remarks on the factorization choice. It can be discussed tomorrow if needed.

Best

Algiane · 2023-10-23T08:02:22Z

src/common/hash.c

+ * \return 1 if success, 0 if fail (edge is not found).
+ *
+ * Update the index of the new point stored along the edge \f$[a;b]\f$
+ * If ls mode in ParMmg:: update the index of the new point in internal edge communicator;


Typo: "::" -> ":"

Algiane · 2023-10-23T08:06:10Z

src/common/hash.c

+
+  return 0;
+}
+


Will this function be used in Mmg? If not, I think that we should move it into ParMmg (and rename it PMMG_hashUpdate_all) with the adding of a remark in the MMG5_hashUpdate function saying that an equivalent function that updates both the k and s fields has been added to ParMmg and can be moved into Mmg if we ever need it in Mmg.

MMG5_hashUpdate_all has been removed from mmg and replaced by PMMG_hashUpdate_all in ParMmg.

Algiane · 2023-10-23T08:06:58Z

src/common/hash.c

+/**
+ * \param hash pointer toward the hash table of edges.
+ * \param a index of the first extremity of the edge.
+ * \param b index of the second extremity of the edge.
+ * \param k index of new point along the edge [a,b].
+ * \param s If ls mode in ParMmg:: index of new point in internal edge communicator;
+ *          otherwise, the value stored in variable s.
+ * \return 1 if success, 0 if fail (edge is not found).
+ *
+ * Find the index of the new point stored along the edge \f$[a;b]\f$
+ * If ls mode in ParMmg:: find the index of the new point in internal edge communicator;
+ * otherwise, find the value stored in variable s.
+ *
+ */
+MMG5_int MMG5_hashGet_all(MMG5_Hash *hash,MMG5_int a,MMG5_int b,MMG5_int *k,MMG5_int *s) {
+  MMG5_hedge  *ph;
+  MMG5_int    key;
+  MMG5_int    ia,ib;
+
+  if ( !hash->item ) return 0;
+
+  ia  = MG_MIN(a,b);
+  ib  = MG_MAX(a,b);
+  key = (MMG5_KA*(int64_t)ia + MMG5_KB*(int64_t)ib) % hash->siz;
+  ph  = &hash->item[key];
+
+  if ( !ph->a )  return 0;
+  if ( ph->a == ia && ph->b == ib )  {
+    *k = ph->k;
+    *s = ph->s;
+    return 1;
+  }
+  while ( ph->nxt ) {
+    ph = &hash->item[ph->nxt];
+    if ( ph->a == ia && ph->b == ib ) {
+      *k = ph->k;
+      *s = ph->s;
+      return 1;
+    }
+  }
+  return 0;
+}
+


Same as previous comment: If not used in Mmg, I think that it is better to move this inside ParMmg.

MMG5_hashGet_all has been removed from mmg and replaced by PMMG_hashGet_all in ParMmg.

Algiane · 2023-10-23T08:08:41Z

src/common/mmgcommon_private.h

 MMG5_int      MMG5_hashFace(MMG5_pMesh,MMG5_Hash*,MMG5_int,MMG5_int,MMG5_int,MMG5_int);
 int           MMG5_hashEdge(MMG5_pMesh mesh,MMG5_Hash *hash,MMG5_int a,MMG5_int b,MMG5_int k);
 int           MMG5_hashUpdate(MMG5_Hash *hash,MMG5_int a,MMG5_int b,MMG5_int k);
+ int           MMG5_hashUpdate_all(MMG5_Hash *hash,MMG5_int a,MMG5_int b,MMG5_int k,MMG5_int s);


To clean if we choose to move the function defs into ParMmg

Algiane · 2023-10-23T08:09:01Z

src/common/mmgcommon_private.h

+ int           MMG5_hashUpdate_all(MMG5_Hash *hash,MMG5_int a,MMG5_int b,MMG5_int k,MMG5_int s);
 int           MMG5_hashEdgeTag(MMG5_pMesh mesh,MMG5_Hash *hash,MMG5_int a,MMG5_int b,int16_t k);
 MMG5_int      MMG5_hashGet(MMG5_Hash *hash,MMG5_int a,MMG5_int b);
+ MMG5_int      MMG5_hashGet_all(MMG5_Hash *hash,MMG5_int a,MMG5_int b,MMG5_int *k,MMG5_int *s);


To clean if we choose to move the function defs into ParMmg

Algiane · 2023-10-23T08:19:35Z

src/mmg3d/split_3d.c

 *
 */
-int MMG5_split1(MMG5_pMesh mesh,MMG5_pSol met,MMG5_int k,MMG5_int vx[6],int8_t metRidTyp) {
+int MMG5_split1_globNum(MMG5_pMesh mesh,MMG5_pSol met,MMG5_int k,MMG5_int vx[6],MMG5_int vGlobNum[4],int8_t metRidTyp) {


If vGlobNum is not used (because there are no ambiguous configs in the face splitting), I think that we can avoid this function redefinition and just let the old MMG5_split1 function unchanged.

Ok, MMG5_split1 has been kept as it was.

Algiane · 2023-10-23T08:21:40Z

src/mmg3d/split_3d.c


 /**
 * \param flag flag to detect the splitting configuration
+ * \param v indices of the tetra nodes


* \param v indices of the tetra nodes (global node indices if called from ParMmg)

Algiane · 2023-10-23T08:22:56Z

src/mmg3d/split_3d.c

+ * \param met pointer toward the metric structure.
+ * \param k index of element to split.
+ * \param vx \f$vx[i]\f$ is the index of the point to add on the edge \a i.
+ * \param vGlobNum vertices indices of the tetra k.


* \param vGlobNum vertices indices of the tetra k (global vertices indices if we come from ParMmg)

Algiane · 2023-10-23T08:35:21Z

src/mmg3d/libmmg3d_private.h

+void MMG3D_split1_cfg(MMG5_int flag,uint8_t *tau,const uint8_t **taued);
 int  MMG3D_split1_sim(MMG5_pMesh mesh,MMG5_pSol met,MMG5_int k,MMG5_int vx[6]);
 int  MMG5_split1(MMG5_pMesh mesh,MMG5_pSol met,MMG5_int k,MMG5_int vx[6],int8_t metRidTyp);
+int  MMG5_split1_globNum(MMG5_pMesh mesh,MMG5_pSol met,MMG5_int k,MMG5_int vx[6],MMG5_int vGlobNum[4],int8_t metRidTyp);


If we don't need the global numbering for this function, I think that we can remove the MMG5_split1_globNum function (for sake of readability).

Algiane · 2023-11-07T07:52:28Z

Thanks

laetitia-m added 19 commits August 4, 2023 14:26

Add hash function to update and get hash.item[].s - used in parmmg to…

f384c22

… update communicator

Add split using global vertex indices - for parmmg convenience

241a629

Merge branch 'develop' into feature/split-vertices

2b034f3

(1) Remove functions to get and update s variable in hash table and (…

7a7aea9

…2) Add 2 new functions to get and update k and s variables at the same time in hash table

Modification of split functions using global indices

c92871b

Progress in split using global num

c8fb078

Modify split function

434e42d

Split using global vertices

57d8dcb

update gloabl split

0897b02

Modify split to have config function and using global numerotation

8904a87

Modify print in split for debug convenience

496fa4b

Take into account Algiane feedbacks

3e8f26c

Clean comments for debug splits occuring in ParMmg

dd39ad0

Wrap the splits function used in ls for ParMmg convenience (to be abl…

e669076

…e to use global indices of nodes to determine the split configuration)

Improve headers of functions

a0ed52e

Remove spaces between arguments

a963ebb

Reverse formatting in split4op to comply with old formatting

6b67fde

Modify type of function MMG5_hashGet_all

5485e53

Fix type inconsistencies in split3d

36584a2

Algiane self-assigned this Oct 23, 2023

Algiane self-requested a review October 23, 2023 07:59

Algiane reviewed Oct 23, 2023

View reviewed changes

Modify PR to take into account remarks and feedback

e418bf2

Algiane added kind: refactoring needs refactoring part: mmg3d mmg3d specific labels Nov 7, 2023

Algiane merged commit 62340b7 into MmgTools:develop Nov 7, 2023

laetitia-m deleted the feature/split-vertices branch November 7, 2023 10:41

Uh oh!

Conversation

laetitia-m commented Oct 16, 2023 • edited by Algiane Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Algiane left a comment

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!

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!

Algiane commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

laetitia-m commented Oct 16, 2023 •

edited by Algiane

Loading