Parallelization#60
Conversation
|
I guess the failing test is because libROM PR #316 has not been merged yet, right? |
dreamer2368
left a comment
There was a problem hiding this comment.
@dylan-copeland , I've gone through your PR. While I appreciate your efforts on this parallelization work, I'm a bit cautious about merging it into main. Most importantly, this intermediate stage doesn't keep the compatibility for other solvers that are not yet parallelized.
I suggest to keep this branch until the parallelization is done for the entire framework. Another minor suggestion is to have a short definition/description on newly introduced variables for parallel workflow. Also please see the other comments.
| // Receive topology info | ||
| numSub = topol_data.numSub; | ||
| numSubLoc = topol_handler->GetNumLocalSubdomains(); | ||
| numSubStored = meshes.Size(); |
There was a problem hiding this comment.
I don't see the difference between numSubLoc and numSubStored. Based on this initialization, they should be identical. Can you only use numSubLoc, or explain the difference in PR or in the class definition?
| HYPRE_BigInt sys_glob_size; | ||
| HYPRE_BigInt sys_row_starts[2]; | ||
| HypreParMatrix *globalMat_hypre = NULL; | ||
| HypreParMatrix *systemOp_hypre = NULL; |
There was a problem hiding this comment.
This doesn't seem to get deleted at the destruction of PoissonSolver.
| break; | ||
| } | ||
| Array<int> &bdr_marker = *bfnfi_marker[k]; | ||
| /* |
There was a problem hiding this comment.
why is this commented out?
| break; | ||
| } | ||
| Array<int> &bdr_marker = *bfnfi_marker[k]; | ||
| /* |
| else | ||
| { | ||
| Array<int> &bdr_marker = *bfnfi_marker[k]; | ||
| /* |
| int midx = -1; | ||
| int vidx = (separate_variable) ? b % num_var : 0; | ||
| for (int m = 0; m < numSub; m++) | ||
| for (int m = 0; m < numSubLoc; m++) |
There was a problem hiding this comment.
topol_handler->GetMeshType(m) seems to take global index, not local index. Is this still valid?
| int local_dim = CAROM::split_dimension(dim_ref_basis[k], MPI_COMM_WORLD); | ||
| basis_reader = new CAROM::BasisReader(basis_name + basis_tags[k].print(), CAROM::Database::formats::HDF5_MPIO, local_dim); | ||
| int local_dim = dim_ref_basis[k]; | ||
| basis_reader = new CAROM::BasisReader(basis_name + basis_tags[k].print() + ".000000", CAROM::Database::formats::HDF5, local_dim, MPI_COMM_NULL); |
There was a problem hiding this comment.
I'm not objecting this convention change. However, until we fully implement parallel setting on all solvers, the code needs to be compatible in both cases.
| for (int mm = 0; mm < numSubLoc; mm++) | ||
| { | ||
| const int m = mm + ossub; | ||
| const int m_global = topol_handler->GlobalSubdomainIndex(mm); |
There was a problem hiding this comment.
what is the difference between m and m_global? If they are identical, can we choose m_global only?
| localBlocks[0] = bsum; | ||
| } | ||
|
|
||
| MFEM_VERIFY(sum == gsize && bsum == num_rom_blocks, ""); |
There was a problem hiding this comment.
Can we print out a clear error message for this?
| { | ||
| if (!sample_generator->IsMyJob(s)) continue; | ||
| if (!sample_generator->IsMyJob(s)) { | ||
| s++; |
| systemOp->SetBlock(0,1, Bt); | ||
| systemOp->SetBlock(1,0, B); | ||
| } | ||
| else |
There was a problem hiding this comment.
So parallel path doesn't seem to set M, B, Bt, and systemOp, which are used in !direct_solve case in StokesSolver::Solve. Can we at lease raise an error if parallel iterative case is set up?
| Vector pres_view(sol_byvar, vblock_offsets[1], vblock_offsets[2] - vblock_offsets[1]); | ||
|
|
||
| // TODO(kevin): parallelization. | ||
| double tmp = pres_view.Sum() / pres_view.Size(); |
There was a problem hiding this comment.
I think this part needs a proper average calculation in parallel path.
| if (set_oper) mumps->SetOperator(*systemOp_hypre); | ||
| } | ||
|
|
||
| void StokesSolver::SetupMUMPSSolverParallel() |
There was a problem hiding this comment.
do we not need delete commands similar to the serial version?
|
|
||
| pmMat = new BlockMatrix(p_offsets); | ||
| for (int m = 0; m < numSub; m++) | ||
| for (int m = 0; m < numSubStored; m++) |
There was a problem hiding this comment.
This may not have run in parallel case since it is only used for iterative solver. the pressure mass matrix needs to be properly set up in parallel case. If the PR does not support iterative solver, then raise an error if this function is called at parallel case.
Parallelization of ROM and FOM linear system assembly and solution, using
HypreParMatrix.This is the first step in parallelizing the library, focused on ComponentTopologyHandler and the Stokes and Poisson examples. Some things still remain to be parallelized and optimized: