MueLu coarse coordinates #3053
Merged
MueLu coarse coordinates #3053
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
mhoemmen
reviewed
Jul 3, 2018
|
@mhoemmen Yes, you could. The idea was to have one single typedef for a real-valued multivector, instead of having it in every single file. In the unlikely scenario that we want to change the type for coordinates, we have to edit a single file, instead of grepping through everything for hard-coded doubles.. |
|
@cgcgcg So why not have a typedef just for the type of coordinates, instead of having a typedef for the type of a MultiVector containing coordinates? A typedef just for the type of a coordinate would be orthogonal to the other template parameters. |
cgcgcg
reviewed
Jul 3, 2018
| } | ||
| #else | ||
| RCP<CoupledAggregationFactory> CoupledAggFact2 = rcp( new CoupledAggregationFactory() ); | ||
| CoupledAggFact2 = rcp( new CoupledAggregationFactory() ); |
There was a problem hiding this comment.
I guess this is superfluous?
| #include "MueLu_CoalesceDropFactory_kokkos.hpp" | ||
| // #include "MueLu_CoarseMapFactory_kokkos.hpp" | ||
| // #include "MueLu_CoordinatesTransferFactory_kokkos.hpp" | ||
| // #include "MueLu_NullspaceFactory_kokkos.hpp" |
There was a problem hiding this comment.
How come we don't need NullspaceFactory_kokkos, but the regular NullspaceFactory?
|
@mhoemmen True, we could have done that. I prefer this, since we like to use the shorthand |
|
Would you ever want to support multiple coordinate types in a single MueLu build? If not, why not use a typedef instead of a template parameter? Compare: Tpetra has a LocalOrdinal type template parameter, but it's only ever been tested with |
|
@mhoemmen, I hear you but honestly based on how this was handled so far I do not feel confident that people will remember to do typedef appropriately, the RealValuedMultiVector seems easier to uniformize the handling of coordinates even if it is not the most elegant. |
|
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.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
@lucbv I would like to push back a little bit on this. Intel debug builds literally don't work for SPARC any more because the linker overflows The lesson learned in Tpetra is that if you give users a template parameter, they will put anything in there that compiles. GlobalOrdinal should never have been a template parameter. Now we find ourselves with If you expose a new template parameter without constraining it, you will find yourself supporting many more possible template parameter values than you intended. Literally this is how I would fix this problem: // some MueLu main header file
// Type of a coordinate
using coordinate_type = double;
// Type of a MultiVector of coordinates
template<class MultiVectorType>
using coordinate_multivector_type = Xpetra::MultiVector<coordinate_type, typename MultiVectorType::local_ordinal_type, typename MultiVectorType::global_ordinal_type, typename MultiVectorType::node_type>;Now both users and MueLu developers can do this: using XMV = Xpetra::MultiVector<Scalar, LO, GO, Node>;
XMV X (...);
coordinate_multivector_type<XMV> coords (...); |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)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_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)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_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
lucbv
added
the
AT: WIP
|
@mhoemmen, I am working on this, the level of pain is at about 120%, I hope this will decrease our build time/space by a reasonable amount. |
lucbv
removed
the
AT: WIP
|
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.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
@lucbv wrote:
That's not so bad for this solver stack ;-)
Me too, though I think it's still worth having orthogonal template parameters, even if there is no build time or size improvement.
I agree that we should change this. I filed an issue for this: #3077 |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
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.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)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_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)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_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
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... |
|
@mhoemmen can you look at this again, I think this satisfies your requirement now, It also passes tests, so it should be ready to be merge. |
mhoemmen
reviewed
Jul 12, 2018
| @@ -61,6 +61,7 @@ namespace Galeri { | |||
| template <typename Scalar, typename LocalOrdinal, typename GlobalOrdinal, typename Map, typename Matrix, typename MultiVector> | |||
| class Elasticity2DProblem : public Problem<Map,Matrix,MultiVector> { | |||
| public: | |||
| using RealValuedMultiVector = typename Problem<Map,Matrix,MultiVector>::RealValuedMultiVector; | |||
There was a problem hiding this comment.
I much prefer this -- thanks!
| typedef Teuchos::ScalarTraits<Scalar> TST; | ||
|
|
||
| Teuchos::ArrayRCP<SC> x = this->Coords_->getDataNonConst(0); | ||
| Teuchos::ArrayRCP<SC> y = this->Coords_->getDataNonConst(1); | ||
| Teuchos::ArrayRCP<real_type> x = this->Coords_->getDataNonConst(0); |
There was a problem hiding this comment.
For later (not for this PR), replace use of getData and getDataNonConst to use Kokkos data structures.
| @@ -370,8 +375,8 @@ namespace Galeri { | |||
| } | |||
|
|
|||
| // Calculate center | |||
| SC cx = this->Coords_->getVector(0)->meanValue(); | |||
| SC cy = this->Coords_->getVector(1)->meanValue(); | |||
| real_type cx = this->Coords_->getVector(0)->meanValue(); | |||
There was a problem hiding this comment.
For later (not this PR), consider merging these two all-reduces into one.
| @@ -61,12 +61,12 @@ namespace Galeri { | |||
|
|
|||
| namespace Xpetra { | |||
|
|
|||
| template <typename Scalar, typename LocalOrdinal, typename GlobalOrdinal, typename Map, typename Matrix, typename MultiVector> | |||
| class HelmholtzFEM2DProblem : public Problem_Helmholtz<Map,Matrix,MultiVector> { | |||
| template <typename Scalar, typename LocalOrdinal, typename GlobalOrdinal, typename Map, typename Matrix, typename MultiVector, typename RealValuedMultiVector> | |||
There was a problem hiding this comment.
Why does HelmholtzFEM2DProblem still take a RealValuedMultiVector template parameter?
There was a problem hiding this comment.
Good catch, this only gets compiled with MueLu_ENABLE_Experimental=ON, I'll take care of this.
| @@ -57,16 +57,16 @@ namespace Galeri { | |||
| namespace Xpetra { | |||
|
|
|||
| // ============================================= Helmholtz1D ============================================= | |||
| template <typename Scalar, typename LocalOrdinal, typename GlobalOrdinal, typename Map, typename Matrix, typename MultiVector> | |||
| class Helmholtz1DProblem : public Problem_Helmholtz<Map,Matrix,MultiVector> { | |||
| template <typename Scalar, typename LocalOrdinal, typename GlobalOrdinal, typename Map, typename Matrix, typename MultiVector, typename RealValuedMultiVector> | |||
There was a problem hiding this comment.
Why does HelmholtzFEM1DProblem still take a RealValuedMultiVector template parameter?
|
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... |
lucbv
force-pushed
the
MueLu_Coarse_Coordinates
branch
from
4c3a1c6
to
2d68cf9
Compare
PTentative is now building the coarse coordinates by default which simplifies the automatic handling of coordinates transfer. Also fixing a bunch of unit-test factories logic when kokkos is enabled.
|
@mhoemmen, alright I modified the Helmholtz tests and I did a build with |
|
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.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)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_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
mhoemmen
approved these changes
Jul 13, 2018
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)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_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
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.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
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' - Pull Request MUST BE MERGED MANUALLY BY Project Team - Master Automerge is disabled (in .cfg file) |
2 similar comments
|
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - Master Automerge is disabled (in .cfg file) |
|
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - Master Automerge is disabled (in .cfg file) |
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.
@trilinos/muelu
Description
Modifying the creation of coarse coordinates from CoordinateTransferFactory to prolongators.
Motivation and Context
This avoids having multiple intricate algorithm in the transfer factory when the prolongators already implement most of the work.
Related Issues
How Has This Been Tested?
All unit-test requiring coordinates to be injected in MueLu are still passing
Checklist