ShyLU: Fixed test link errors #5618 #5650
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With deprecated code off, GO=long long and Epetra off, several ShyLU FROSch tests were failing to link because Xpetra_UseDefaultTypes.hpp hardcoded the default GlobalOrdinal as int. Now, a reasonable default GO is chosen based on Tpetra's default type or Epetra's GO type (preferring Tpetra's).
mhoemmen
reviewed
Aug 2, 2019
|
|
||
| // Choose global ordinal based on enabled underlying libraries | ||
| #ifdef HAVE_XPETRA_TPETRA | ||
| typedef typename Tpetra::Map<>::global_ordinal_type GlobalOrdinal; |
There was a problem hiding this comment.
Brian and I talked about this in person today. I suggested adding #error if Epetra and Tpetra are enabled but Tpetra's default ordinal type is not int. That's already an error in Xpetra and it might already catch that case at configure time, but just in case, it would be good to add a test here.
| namespace Details | ||
| { | ||
| //Check for the default Tpetra GlobalOrdinal (long long) | ||
| #ifdef HAVE_XPETRA_TPETRA |
There was a problem hiding this comment.
See above. Also, perhaps we should consolidate these two "test the macros" things in a single place.
|
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_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Using Repos:
Pull Request Author: brian-kelley |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_2
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
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... |
1 similar comment
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
lucbv
approved these changes
Aug 5, 2019
There was a problem hiding this comment.
@brian-kelley I think this is fine for now but if you want to add some logic regarding @mhoemmen comment that's fine too.
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ lucbv ]! |
|
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... |
|
@brian-kelley Thanks for fixing this and #5637. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed link errors in several ShyLU_DDFROSch tests, where the global ordinal was
unknowingly hardcoded as int. Made Xpetra pick a reasonable default GlobalOrdinal.
@trilinos/xpetra
@trilinos/shylu
Description
Building with Tpetra GO=long long (but not GO=int) caused the failure, because Xpetra structrues were not instantiated with *_int_int_node. The reason why GO was hardcoded to int in ShyLU is because ShyLU used the Xpetra "default types" to define Scalar, LO, GO and NO. In Xpetra_DefaultTypes.hpp, GO was hardcoded to int, regardless of what was enabled in CMake. So this commit adds logic in Xpetra to choose a reasonable default GlobalOrdinal based on what is enabled: if Tpetra is enabled, the default GO of Tpetra is chosen. Then if Epetra 64-bit GO is enabled, long long is used. Otherwise, int is used.
Motivation and Context
This fixes a ShyLU build failure as part of the Tpetra deprecated code removal (#5602).
How Has This Been Tested?
This was built with Tpetra GO=long long enabled, GO=int disabled, Epetra disabled, and deprecated code disabled. Most tests in FROSch failed to build before, but now build and run successfully (except for the two in #5617, which were fixed separately).
Checklist
I didn't say that all existing tests passed, because some other fixes (see #5602) for tests in this configuration have not made it to develop branch yet. The tests affected by these changes do pass.