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

Pcu object #388

Open
wants to merge 120 commits into
base: develop
Choose a base branch
from
Open

Pcu object #388

wants to merge 120 commits into from

Conversation

jacobmerson
Copy link
Contributor

No description provided.

jacobmerson and others added 13 commits March 28, 2023 04:14
This commit adds a MPI state that's separate from the global variables
to the internal pcu_mpi interfaces. The user level PCU interface remains
unchanged. However, this change lays the groundwork for PCU2 that will
not make use of global state. This is important for some use cases like
working around the spectrum-mpi comm_dup bug. The removal of global
state also improves testability and modularity of this code.
Throughout the code base it was assumed that the mpi functions were
implemented through pmpi and so no other implementation really could
have been implemented. This change just removes the complexity
from having the vtable.
This commit allows the pcu_mpi to be initialized into a stack variable
rather than allways allocating on the heap.
Copy link
Contributor

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

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

Thank you. Nice work.

I just noticed this was a draft. What is left to do? It looks like CI tests are passing.

@@ -9,7 +9,7 @@ message(STATUS "PCU_COMPRESS: " ${PCU_COMPRESS})

# Package sources
set(SOURCES
pcu.c
pcu.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

extra whitespace?


# Package headers
set(HEADERS
PCU.h
pcu_io.h
pcu_io.h
Copy link
Contributor

Choose a reason for hiding this comment

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

extra whitespace?

pcu/pcu.cc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@jacobmerson
Copy link
Contributor Author

Thank you. Nice work.

I just noticed this was a draft. What is left to do? It looks like CI tests are passing.

I think it's good I just figured we may want to discuss/make sure we are comfortable before merging.

@cwsmith
Copy link
Contributor

cwsmith commented Mar 31, 2023

meeting notes

  • test C++ interface with multiscale code
  • test large scale (100s) adapt case
  • merge to develop
  • watch nightly test for merge to master
  • ask friendly users who run at scale to pull master
  • eventually consider allowing a mesh object owning a pcu instance

jacobmerson and others added 11 commits January 10, 2024 22:21
This commit adds a MPI state that's separate from the global variables
to the internal pcu_mpi interfaces. The user level PCU interface remains
unchanged. However, this change lays the groundwork for PCU2 that will
not make use of global state. This is important for some use cases like
working around the spectrum-mpi comm_dup bug. The removal of global
state also improves testability and modularity of this code.
Throughout the code base it was assumed that the mpi functions were
implemented through pmpi and so no other implementation really could
have been implemented. This change just removes the complexity
from having the vtable.
This commit allows the pcu_mpi to be initialized into a stack variable
rather than allways allocating on the heap.
@jacobmerson jacobmerson mentioned this pull request Apr 29, 2024
@jacobmerson jacobmerson requested a review from bobpaw April 29, 2024 14:31
PhastaSharing* shr = new PhastaSharing(m);
PCU_Comm_Begin();
PhastaSharing shr(m);
m->getPCU()->Begin();
apf::MeshIterator* it = m->begin(dim);
apf::MeshEntity* v;
while ((v = m->iterate(it))) {

Choose a reason for hiding this comment

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

line 106 and 114 should be shr.

@jacobmerson jacobmerson marked this pull request as ready for review May 6, 2024 16:46
@jacobmerson jacobmerson requested a review from cwsmith May 6, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants