Bddc #3738
Bddc #3738
Conversation
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Using Repos:
Pull Request Author: crdohrm |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
There was a problem hiding this comment.
Thanks Clark! There's a lot of code to review and I haven't looked at it all. I noticed some issues that would likely break the build with default settings, and pointed those out. I also commented on some stylistic issues. I would be happy to schedule a meeting to work through the code, just not this week :-)
The problem with the build issues is that you won't see them in PR testing, because this package may not / likely is not enabled in PR testing. Just building in situ in (say) Sierra won't necessarily identify those build issues, since Sierra does not use default settings for things like the default GlobalOrdinal type.
| using Teuchos::RCP; | ||
|
|
||
| typedef int LO; // Local Ordinal | ||
| typedef long GO; // Global Ordinal |
There was a problem hiding this comment.
If you're using Tpetra in this example, you would need to check whether GO=long is enabled. If you don't really depend on GO=long, it would be better just to depend on Tpetra's default global ordinal type:
typedef Tpetra::Map<>::global_ordinal_type GO;There was a problem hiding this comment.
Followed your suggestion and am now using Tpetra's default global ordinal type.
There was a problem hiding this comment.
Here is how you can ensure that you always get a 64-bit global ordinal type:
// Your application prefers long, though I don't
// necessarily agree that this is a good idea.
#if defined(HAVE_TPETRA_INST_INT_LONG)
using GO = long;
#elif defined(HAVE_TPETRA_INST_INT_LONG_LONG)
using GO = long long;
#elif defined(HAVE_TPETRA_INST_INT_UNSIGNED_LONG)
using GO = unsigned long;
# error "Tpetra does not enable one of the possibly 64-bit global ordinal types."
#endif
static_assert (sizeof (GO) >= size_t (8), "GO is not a 64-bit type.");There was a problem hiding this comment.
Once Tpetra finishes deprecation and removal of the GlobalOrdinal template parameter, you should be able to remove the macros and just have one typedef for GO and a static_assert. The typedef for GO would look like this: using GO = Tpetra::Map::global_ordinal_type;. Right now, since the template parameters are still there, you could write using GO = Tpetra::Map<>::global_ordinal_type;.
| LAPACK.STEQR(COMPZ, numRows, D, E, Z, LDZ, WORK, &INFO); | ||
| assert (INFO == 0); | ||
| BDDC_TEST_FOR_EXCEPTION(INFO != 0, std::runtime_error, "STEQR error"); |
There was a problem hiding this comment.
Consider not just throwing, but collecting up some error info to propagate on the next reduction. What if some processes throw and others don't? I don't insist on this, since many other solvers don't do it, but it's something to think about.
There was a problem hiding this comment.
Yes, I'll think about how to do this better.
There was a problem hiding this comment.
This is hard, but just something to think about for the future.
| pComm->m_distributor->getProcsFrom()); | ||
| m_sourceLength = pComm->m_sourceLength; | ||
| m_targetLength = pComm->m_targetLength; | ||
| m_sendCount = pComm->m_sendCount; |
There was a problem hiding this comment.
I'm not keen on this "break encapsulation by pulling all the fields out of the input" design pattern.
There was a problem hiding this comment.
Did you have in mind using getter functions to not break encapsulation? I could do that once we have a chance to discuss.
There was a problem hiding this comment.
@crdohrm Let's talk more about this in the coming week.
There was a problem hiding this comment.
Sounds good. I should be in this week Monday through Thursday.
There was a problem hiding this comment.
I think I understand. This is a copy constructor that copies everything except the MPI_Comm and the Distributor. Why not just write a copy constructor? You don't need to take the input pComm by RCP because you don't keep it.
There was a problem hiding this comment.
This code just deep copies the input pComm. Take the input pComm by const reference instead of by RCP.
There was a problem hiding this comment.
We discussed this over the phone.
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
There was a problem hiding this comment.
This a huge commit. I couldn't review it line by line, but I have some comments on places wherever I saw some things that could be changed. It would be nice to avoid calling METIS/ParMETIS directly and introduce an optional dependency on Zoltan2. This doesn't have to be this commit, but something to come back and do it later.
| #ifdef _OPENMP | ||
| #include "omp.h" | ||
| #else | ||
| #define omp_get_max_threads() 1 |
There was a problem hiding this comment.
Is this right to define this to be 1 ?
There was a problem hiding this comment.
I shouldn't speak for Clark, but I think the point of this was a quick hack to make OpenMP code compile without OpenMP support. It would be better to encapsulate something like this in a function, if the code isn't suitable for translation to Kokkos.
There was a problem hiding this comment.
The subject code was not needed and was removed.
| #endif | ||
|
|
||
| //#define TENSORPRODUCTBDDC | ||
| #ifdef TENSORPRODUCTBDDC |
There was a problem hiding this comment.
Is there a test for this option ?
There was a problem hiding this comment.
Not yet. This is a new capability for higher-order elements that would be included in a future pull request.
| // NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS | ||
| // SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| // | ||
| // Questions? Contact Michael A. Heroux ([email protected]) |
There was a problem hiding this comment.
Please change yourself to contact. We have moved away from this convention of using @maherou as the contact for all the files.
There was a problem hiding this comment.
Done. Changed all occurrences of Micahel A. Heroux to yours truly.
| @@ -49,8 +49,9 @@ | |||
| #include <complex> | |||
There was a problem hiding this comment.
Does this have to be part of the main library or can it be an utility for the tests ? Also it would be nice to use Belos so we could reuse the different solver there.
There was a problem hiding this comment.
Right now I'm using my own Krylov solvers (PCG and GCR) to actually solve a linear system, so they would also be needed by those using BDDC to solve linear systems. Yes, I should consider moving away from these to Belos since they may already be high performance for GPUs.
| #endif | ||
|
|
||
| //#define NODALAMGBDDC | ||
| #ifdef NODALAMGBDDC |
There was a problem hiding this comment.
Is there a way to build without NODALMGBDDC ? I thought this was always built by default ?
There was a problem hiding this comment.
The capabilities in NodalAMG will be included in a future pull request. Right now the build would not include NodalAMG since NODALAMGBDDC is not defined.
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
1 similar comment
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
mhoemmen
added
the
AT: WIP
|
Added "AT: WIP" label per @crdohrm 's request. |
|
Don't know the next step for me (Clark) right now. |
srajama1
removed
the
AT: WIP
|
Removed the AT:WIP label, so PR tester can start testing. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
crdohrm
added
the
AT: WIP
crdohrm
added
AT: RETEST
trilinos-autotester
removed
the
AT: STALE
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Using Repos:
Pull Request Author: crdohrm |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 1741 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3 # 2306 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 1967 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3_SERIAL # 278 (click to expand) |
trilinos-autotester
removed
the
AT: RETEST
…fore). We'll see how the pull requested testing goes.
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Using Repos:
Pull Request Author: crdohrm |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
4 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mhoemmen ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
@trilinos/
Description
Motivation and Context
How Has This Been Tested?
Checklist