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

Add and calculate coordinate container for off-rank neighbor nodes. #1081

Merged
merged 5 commits into from
Jul 7, 2021

Conversation

RyanWollaeger
Copy link
Contributor

@RyanWollaeger RyanWollaeger commented Jun 30, 2021

Background

  • The off-rank Dual_Ghost_Layout container can now see node indices local to the off-rank meshes that share a face with the MPI rank boundary nodes (effectively one node-layer into the off-rank mesh).
  • However, to expedite computation over node-based stencils, the coordinate values of the nodes can be obtained with a similar communication pattern used to get the node indices on the off-rank mesh.

Purpose of Pull Request

Description of changes

  • Create node-coordinate map that is 1-to-1 with map to node indexes in dual ghost layout.
  • Add specializations on double of allgatherv and determinate_allgatherv.
  • Use double determinate_allgatherv to get ghost neighbor node coordinates.
  • Use consistent loop variable names in loops serializing data for allgatherv.
  • Add tests of ghost coordinate values to tstDraco_Mesh_DD unit test.
  • Add mesh decomposition ASCII art schematics to tstDraco_Mesh_DD tests.

Note: this performs an additional allgatherv of four doubles between ranks
(two doubles for each coordinate adjacent to the node and its neighbor cell).

Status

+ Create node-coordinate map that is 1-to-1 with map to node indexes in dual ghost layout.
+ Add specializations on double of allgatherv and determinate_allgatherv.
+ Use double determinate_allgatherv to get ghost neighbor node coordinates.
+ Use consistent loop variable names in loops serializing data for allgatherv.
+ Add tests of ghost coordinate values to tstDraco_Mesh_DD unit test.
+ Add mesh decomposition ASCII art schematics to tstDraco_Mesh_DD tests.

Note: this performs an additional allgatherv of four doubles between ranks
(two doubles for each coordinate adjacent to the node and its neighbor cell).
@RyanWollaeger
Copy link
Contributor Author

Forgot about the double == double error; will replace these with soft_equiv.

@clevelam
Copy link
Collaborator

@brryan This is another good PR to track. Ryan is ensuring we have access to the full dual mesh which will be required for DDMC on unstructured mesh.

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #1081 (c17e7c4) into develop (d376f66) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           develop   #1081   +/-   ##
=======================================
  Coverage     88.6%   88.7%           
=======================================
  Files          374     374           
  Lines        18579   18601   +22     
=======================================
+ Hits         16479   16505   +26     
+ Misses        2100    2096    -4     

@KineticTheory
Copy link
Collaborator

The Appveyor CI failure isn't related to this PR. Are you able to restart it? If not, I'll build/test this PR in my environment to confirm that it is clean. Let me know...

@KineticTheory KineticTheory added this to the Draco-7_12_0 milestone Jul 6, 2021
Copy link
Collaborator

@KineticTheory KineticTheory left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of comments.

src/c4/gatherv_pt.cc Show resolved Hide resolved
src/mesh/Draco_Mesh.cc Outdated Show resolved Hide resolved
src/mesh/test/tstDraco_Mesh_DD.cc Show resolved Hide resolved
@RyanWollaeger
Copy link
Contributor Author

@KineticTheory I can't seem to restart it from GitHub (I might be missing something however).
Tried creating an Appveyor account, but this did not seem to permit me to restart it (could not see how...).

@KineticTheory
Copy link
Collaborator

KineticTheory commented Jul 6, 2021

@KineticTheory I can't seem to restart it from GitHub (I might be missing something however).
Tried creating an Appveyor account, but this did not seem to permit me to restart it (could not see how...).

I was able to restart it. I forgot that I needed to log into the Appveyor system. Let's see what it reports.

It still fails with not an archive error. But I think I know the cause. I'll need to make some changes to re-activate Appveyor CI. For this PR, I'll test locally.

@KineticTheory
Copy link
Collaborator

@RyanWollaeger BTW - the MSVC builds/tests are clean.

I'll put in a PR to fix the broken Appveyor CI.

+ Replace push_back with emplace_back in dual ghost map generation.
+ Add unit tests of [in]determinate_allgatherv specializations.
+ Fix else branch (missing) in logic for other gatherv unit tests.
@KineticTheory
Copy link
Collaborator

@KineticTheory I can't seem to restart it from GitHub (I might be missing something however).
Tried creating an Appveyor account, but this did not seem to permit me to restart it (could not see how...).

I was able to restart it. I forgot that I needed to log into the Appveyor system. Let's see what it reports.

It still fails with not an archive error. But I think I know the cause. I'll need to make some changes to re-activate Appveyor CI. For this PR, I'll test locally.

I was able to fix the Appveyor issue. It seems to be running now.

@KineticTheory KineticTheory merged commit ac1d317 into lanl:develop Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants