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

(1) Change tag type (int -> uint) and (2) creation of the overlap #123

Merged
merged 42 commits into from
Oct 24, 2024

Conversation

laetitia-m
Copy link
Contributor

@laetitia-m laetitia-m commented Aug 30, 2024

  1. The type of tag was signed integer int, and in now unsigned integer uint. This was needed to be able to add the extra tag MG_OVERLAP
  2. Creation of the overlap. The overlap consists of one extra layer of tetra (and needed points) coming from neighbour partitions.
  • Add structure PMMG_Overlap in file src/libparmmgtypes.h
  • In file mpi_pmmg.h: add MPI_OVERLAP_TAG for tag used to transfer data with mpi
  • In file overlap_pmmg.c: functions to create PMMG_create_overlap and delete PMMG_delete_overlap the overlap between partitions are added.
  • The overlap is creating and deleting before and after the snap val PMMG_snpval_ls in the function PMMG_ls in file ls_pmmg.c
  • Add overlap ci tests in cmake/testing/pmmg_tests.cmake
  • In file libparmmg.c, function PMMG_Compute_verticesGloNum: skip points tagged MG_OVERLAP

Following pictures illustrate the overlap on a 2D case:

Picture3

Picture6

… Adding tetra works but then fail in the analysis for now
Copy link
Member

@Algiane Algiane left a comment

Choose a reason for hiding this comment

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

@coprigent : few unformatted notes that follows the discussion we had this morning. I let you read the code and take over from Laetitia to end this work

src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/libparmmgtypes.h Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 97.57085% with 6 lines in your changes missing coverage. Please review.

Project coverage is 63.76%. Comparing base (8e03c1f) to head (57c0a6f).
Report is 43 commits behind head on develop.

Files with missing lines Patch % Lines
src/ls_pmmg.c 57.14% 3 Missing ⚠️
src/overlap_pmmg.c 99.15% 1 Missing and 1 partial ⚠️
src/debug_pmmg.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #123      +/-   ##
===========================================
+ Coverage    63.32%   63.76%   +0.44%     
===========================================
  Files           46       47       +1     
  Lines        18952    19194     +242     
  Branches      3540     3573      +33     
===========================================
+ Hits         12001    12240     +239     
- Misses        6027     6030       +3     
  Partials       924      924              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Algiane Algiane added kind: enhancement enhancement to an existing feature part: parmmg specific to parmmg code part part: ls discretization isovalue mode specificty part: analysis labels Sep 13, 2024
Copy link
Member

@Algiane Algiane left a comment

Choose a reason for hiding this comment

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

Thanks,

The works seems great but it is not easy to understand for an external reader. I think we can:

  • Add a 2D relevant picture of the overlap to illustrate the PR an describes:

    • which points os MG_OVERLAP, which is not for each overlap;
    • what the algo does on each type of point (interface points are not transferred, overlap points belonging to only 1 overlap are simply transferred, special (which?) treatment of points belonging to more than 1 overlap)
  • For comments inside the code: maybe an overview of the algo and loops before each loop will help to understand. For example:

    • point beloning to 2 or more communicators are transfered by the first communacator that needs to send it. Then, we perform a double loop to match the PARBDY points that have been communicated as overlap and the list of point belonging to other pairs of communicators (get point index from the list of transferred PARBDY points and search this point index in the list of indices of points of external communicators).... (you know better than me ;-) !

src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
/* Search into dataPBDY_AlreadyAdded to seen if it exists already */
for (r=0; r < ndataPBDY_added+1; r++) {
if ( (color_out == dataPBDY_AlreadyAdded[5*r+1]) && (color_ter == dataPBDY_AlreadyAdded[5*r])) {
if ( k == dataPBDY_AlreadyAdded[5*r+2] ) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess that it is mandatory that k==dataPBDY_AlreadyAdded[5*r+2] here no ?
If yes, use an assert, not a if. If not: what is the case where it is not ?

Copy link
Contributor Author

@laetitia-m laetitia-m Oct 16, 2024

Choose a reason for hiding this comment

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

No, this is not mandatory here because (color_out-color_ter) might have 'given' more than one node to color_in.

dataPBDY_AlreadyAdded is constructed such that dataPBDY_AlreadyAdded = [Pout,Pter,rPter,ipnew,ipout...]
dataPBDY_ToRecv is constructed such that [Pter,k,ipout]

Assuming we are working on Pin, the good couple (Pout, Pter) (if ( (color_out == dataPBDY_AlreadyAdded[5*r+1]) && (color_ter == dataPBDY_AlreadyAdded[5*r]))) has been identified, but potentially this couple can have given several nodes to Pin (not only one) such that the location in the internal communicator (k in dataPBDY_ToRecv and rPter in dataPBDY_AlreadyAdded) is not necessary the same.

src/overlap_pmmg.c Show resolved Hide resolved
src/overlap_pmmg.c Outdated Show resolved Hide resolved
src/overlap_pmmg.c Show resolved Hide resolved
src/overlap_pmmg.c Show resolved Hide resolved
@Algiane Algiane merged commit d11d85c into MmgTools:develop Oct 24, 2024
22 checks passed
@Algiane
Copy link
Member

Algiane commented Oct 24, 2024

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement enhancement to an existing feature part: analysis part: ls discretization isovalue mode specificty part: parmmg specific to parmmg code part
Development

Successfully merging this pull request may close these issues.

3 participants