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

MueLu: Cut Drop Boundary Nodes Not Behaving As Intended #13378

Closed
NyquilDreams opened this issue Aug 21, 2024 · 2 comments · Fixed by #13282
Closed

MueLu: Cut Drop Boundary Nodes Not Behaving As Intended #13378

NyquilDreams opened this issue Aug 21, 2024 · 2 comments · Fixed by #13282
Labels
pkg: MueLu type: bug The primary issue is a bug in Trilinos code or tests

Comments

@NyquilDreams
Copy link

Bug Report

@jhux2 @cgcgcg @csiefer2

Description

While testing the Cut Drop Algorithm in MueLu_CoalesceDropFactory_def.hpp using the unit tests in CoalesceDropFactory.cpp, I noticed that the unit tests did not test for a matrix with boundary nodes. I then noticed that the boundaryNodes View in the algorithm was erroneously being accessed using colID (a loop index) instead of col (the local column index).

for (LO colID = 0; colID < (LO)nnz; colID++) {
LO col = indices[colID];
if (row == col) {
drop_vec.emplace_back(zero, one, colID, false);
continue;
}
// Don't aggregate boundaries
if (boundaryNodes[colID]) continue;
typename STS::magnitudeType aiiajj = STS::magnitude(threshold * threshold * ghostedDiagVals[col] * ghostedDiagVals[row]); // eps^2*|a_ii|*|a_jj|
typename STS::magnitudeType aij = STS::magnitude(vals[colID] * vals[colID]); // |a_ij|^2
drop_vec.emplace_back(aij, aiiajj, colID, false);
}

However, an incorrect assumption is also being made. Boundary nodes is obtained using DetectDirichletRows_kokkos_host which returns a View with a size equal to the number of local rows. This does not account for overlap.

auto boundaryNodes = MueLu::Utilities<SC, LO, GO, NO>::DetectDirichletRows_kokkos_host(*A, dirichletThreshold);

This seems to have been noticed further down in the code. The boundaryNodes View used by the Cut Drop algorithm should make similar changes to the following excerpt. The unit test should also be modified accordingly.

// ghosting of boundary node map
auto boundaryNodes = inputGraph->GetBoundaryNodeMap();
auto boundaryNodesVector = Xpetra::VectorFactory<LocalOrdinal, LocalOrdinal, GlobalOrdinal, Node>::Build(inputGraph->GetDomainMap());
for (size_t i = 0; i < inputGraph->GetNodeNumVertices(); i++)
boundaryNodesVector->getDataNonConst(0)[i] = boundaryNodes[i];
// Xpetra::IO<Scalar,LocalOrdinal,GlobalOrdinal,Node>::Write("boundary",*boundaryNodesVector);
auto boundaryColumnVector = Xpetra::VectorFactory<LocalOrdinal, LocalOrdinal, GlobalOrdinal, Node>::Build(inputGraph->GetImportMap());
boundaryColumnVector->doImport(*boundaryNodesVector, *importer, Xpetra::INSERT);
auto boundaryColumn = boundaryColumnVector->getData(0);

@NyquilDreams NyquilDreams added the type: bug The primary issue is a bug in Trilinos code or tests label Aug 21, 2024
Copy link

Automatic mention of the @trilinos/muelu team

NyquilDreams pushed a commit to NyquilDreams/Trilinos that referenced this issue Aug 24, 2024
Issues listed above have been addressed and unit test has been modified to reflect this.
Column ID check in unit tests might need to be fixed.

Signed-off-by: Ian Halim <[email protected]>
NyquilDreams pushed a commit to NyquilDreams/Trilinos that referenced this issue Oct 1, 2024
Issues listed above have been addressed.
Threshold has been redefined to 1/threshold.
Unit tests have been modified to be more thorough.

Signed-off-by: Ian Halim <[email protected]>
NyquilDreams pushed a commit to NyquilDreams/Trilinos that referenced this issue Oct 11, 2024
Issues listed above have been addressed.
Threshold has been redefined to 1/threshold.
Unit tests have been modified to be more thorough.

Signed-off-by: Ian Halim <[email protected]>
NyquilDreams pushed a commit to NyquilDreams/Trilinos that referenced this issue Oct 17, 2024
Issues listed above have been addressed.
Threshold has been redefined to 1/threshold.
Unit tests have been modified to be more thorough.

Signed-off-by: Ian Halim <[email protected]>
NyquilDreams pushed a commit to NyquilDreams/Trilinos that referenced this issue Oct 31, 2024
Issues listed above have been addressed.
Threshold has been redefined to 1/threshold.
Unit tests have been modified to be more thorough.

Signed-off-by: Ian Halim <[email protected]>
NyquilDreams pushed a commit to NyquilDreams/Trilinos that referenced this issue Nov 1, 2024
Issues listed above have been addressed.
Threshold has been redefined to 1/threshold.
Unit tests have been modified to be more thorough.

Signed-off-by: Ian Halim <[email protected]>
@cgcgcg cgcgcg linked a pull request Nov 8, 2024 that will close this issue
@cgcgcg
Copy link
Contributor

cgcgcg commented Nov 13, 2024

Closed via #13282

@cgcgcg cgcgcg closed this as completed Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: MueLu type: bug The primary issue is a bug in Trilinos code or tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants