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

AggregateNumbering #158

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

Conversation

wrtobin
Copy link
Collaborator

@wrtobin wrtobin commented Jul 23, 2018

Implementation of a read-only numbering type with minimal memory footprint to allow for novel block-structures in the matrices resulting from finite element assembly using these numbering. Mostly for use in the fusion projects, but they can be used as regular a Numbering as well if the user simply wants to reduce memory usage. Aggregation scopes are determined by MPI_Comms passed to the creation functions but the creation is still a globally parallel process across the typical PCU comm.

They inherit from the Numbering type and require only a singe integer or long per node, they are treated by the Mesh as normal Numbering or GlobalNumberings, and are written out to VTK as expected.

They cannot be written to so they cannot be synchronized (though they are synchronized during creation so are globally valid following creation).

Minimal changes have been made outside of the new files to introduce the new concept. Possibly the most impactful is changing several member functions of the NumberingOf class to be virtual member functions.

// the owner rank of unowned entities, so we replace
// it with the passed-in sharing here before globalization
// which requires the rank information
n->setSharing(share);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This may not be required anymore since StaticSharing now supplies ownership information as well.. but may also be the cleaner solution in the end

class AggregateNumberingOf;
typedef AggregateNumberingOf<int> AggNumbering;
typedef AggregateNumberingOf<long> GlobalAggNumbering;
AggNumbering * createAggNumbering(Field * f,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should probably just return Numbering* since AggNumbering inherits from Numbering and the entire point of the class is to be used as a Numbering

MPI_Comm cm,
Sharing * share = NULL);
void destroyAggNumbering(AggNumbering * n);
int getElementNumbers(AggNumbering * n, MeshEntity * e, NewArray<int> & numbers);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah but this needs an AggNumbering specifically rather than just a Numbering?

Copy link
Collaborator Author

@wrtobin wrtobin Jul 23, 2018

Choose a reason for hiding this comment

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

Oh I might be able to just get rid of this since the underlying FieldData is now a UserData<int/long> which should operate correctly in the FieldDataOf<T>::getElementData() function I think.

apf/apfNumbering.cc Show resolved Hide resolved
apf/apfNumbering.h Outdated Show resolved Hide resolved
// init was called
int sz = PCU_Comm_Peers();
frsts.setSize(sz);
MPI_Allgather(&frst,1,MPI_INTEGER,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall if PCU provides Allgather. If not, we should consider adding it to avoid/minimize direct MPI calls. (repeat for the other Allgather calls)

{
int new_ownr = -1;
if(new_cm != MPI_COMM_NULL)
MPI_Group_translate_ranks(pcu_grp,1,&copies[ii].peer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a wrapper call to PCU for this translation?

new_cm = PCU_Get_Comm();
MPI_Comm_group(pcu_cm,&pcu_grp);
if(new_cm != MPI_COMM_NULL)
MPI_Comm_group(new_cm,&new_grp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we easily provide PCU wrappers for the group function and MPI_Group_translate_ranks?

@wrtobin wrtobin force-pushed the wtobin-aggnumbering branch 3 times, most recently from 4c5c125 to e089dae Compare August 6, 2018 15:31
@cwsmith
Copy link
Contributor

cwsmith commented Aug 13, 2018

@TobinW Would you please resolve the conflicts in apf/apfUserData.h? I merged #163 into develop and I think Jacob's clone changes would stay in apfUserData.h, but I'm not 100% sure.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Aug 13, 2018

Sure thing I'll resolve that momentarily.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Aug 13, 2018

Resolved the conflicts, still need to add a test case for the PCU version of Allgather I added before this can be merged.

…g several operations to PCU including PCU_Allgather and rank translation between MPI_Comms as needed in the implementation of AggregateNumberings.
@cwsmith
Copy link
Contributor

cwsmith commented Sep 12, 2018

I hit a few unused-parameter and unused-but-set-variable compilation warnings with gcc7.3 on rhel7. A patch to silence them is attached.
silenceWarnings.patch.gz

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 10, 2019

Once PR #234 is merge I am going to start looking at getting this functionality in as well, at least supporting/exposing implicit fields/numberings so we can supply closed-form representations of the fields/numberings under the hood and still use the 'read-only' version that results for various purposes.

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.

2 participants