{"action":"created","issue":{"url":"https://api.github.com/repos/trilinos/Trilinos/issues/2483","repository_url":"https://api.github.com/repos/trilinos/Trilinos","labels_url":"https://api.github.com/repos/trilinos/Trilinos/issues/2483/labels{/name}","comments_url":"https://api.github.com/repos/trilinos/Trilinos/issues/2483/comments","events_url":"https://api.github.com/repos/trilinos/Trilinos/issues/2483/events","html_url":"https://github.com/trilinos/Trilinos/pull/2483","id":310099094,"number":2483,"title":"Belos: Implement DII for SolverFactory to reduce build time","user":{"login":"MicheldeMessieres","id":12844480,"avatar_url":"https://avatars2.githubusercontent.com/u/12844480?v=4","gravatar_id":"","url":"https://api.github.com/users/MicheldeMessieres","html_url":"https://github.com/MicheldeMessieres","followers_url":"https://api.github.com/users/MicheldeMessieres/followers","following_url":"https://api.github.com/users/MicheldeMessieres/following{/other_user}","gists_url":"https://api.github.com/users/MicheldeMessieres/gists{/gist_id}","starred_url":"https://api.github.com/users/MicheldeMessieres/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MicheldeMessieres/subscriptions","organizations_url":"https://api.github.com/users/MicheldeMessieres/orgs","repos_url":"https://api.github.com/users/MicheldeMessieres/repos","events_url":"https://api.github.com/users/MicheldeMessieres/events{/privacy}","received_events_url":"https://api.github.com/users/MicheldeMessieres/received_events","type":"User","site_admin":false},"labels":[{"id":860599750,"url":"https://api.github.com/repos/trilinos/Trilinos/labels/AT:%20WIP","name":"AT: WIP","color":"00cd00","default":false},{"id":315614807,"url":"https://api.github.com/repos/trilinos/Trilinos/labels/Belos","name":"Belos","color":"5319e7","default":false},{"id":318642986,"url":"https://api.github.com/repos/trilinos/Trilinos/labels/in%20progress","name":"in progress","color":"ededed","default":false}],"state":"open","locked":false,"assignee":{"login":"mhoemmen","id":926747,"avatar_url":"https://avatars0.githubusercontent.com/u/926747?v=4","gravatar_id":"","url":"https://api.github.com/users/mhoemmen","html_url":"https://github.com/mhoemmen","followers_url":"https://api.github.com/users/mhoemmen/followers","following_url":"https://api.github.com/users/mhoemmen/following{/other_user}","gists_url":"https://api.github.com/users/mhoemmen/gists{/gist_id}","starred_url":"https://api.github.com/users/mhoemmen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mhoemmen/subscriptions","organizations_url":"https://api.github.com/users/mhoemmen/orgs","repos_url":"https://api.github.com/users/mhoemmen/repos","events_url":"https://api.github.com/users/mhoemmen/events{/privacy}","received_events_url":"https://api.github.com/users/mhoemmen/received_events","type":"User","site_admin":false},"assignees":[{"login":"mhoemmen","id":926747,"avatar_url":"https://avatars0.githubusercontent.com/u/926747?v=4","gravatar_id":"","url":"https://api.github.com/users/mhoemmen","html_url":"https://github.com/mhoemmen","followers_url":"https://api.github.com/users/mhoemmen/followers","following_url":"https://api.github.com/users/mhoemmen/following{/other_user}","gists_url":"https://api.github.com/users/mhoemmen/gists{/gist_id}","starred_url":"https://api.github.com/users/mhoemmen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mhoemmen/subscriptions","organizations_url":"https://api.github.com/users/mhoemmen/orgs","repos_url":"https://api.github.com/users/mhoemmen/repos","events_url":"https://api.github.com/users/mhoemmen/events{/privacy}","received_events_url":"https://api.github.com/users/mhoemmen/received_events","type":"User","site_admin":false},{"login":"MicheldeMessieres","id":12844480,"avatar_url":"https://avatars2.githubusercontent.com/u/12844480?v=4","gravatar_id":"","url":"https://api.github.com/users/MicheldeMessieres","html_url":"https://github.com/MicheldeMessieres","followers_url":"https://api.github.com/users/MicheldeMessieres/followers","following_url":"https://api.github.com/users/MicheldeMessieres/following{/other_user}","gists_url":"https://api.github.com/users/MicheldeMessieres/gists{/gist_id}","starred_url":"https://api.github.com/users/MicheldeMessieres/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MicheldeMessieres/subscriptions","organizations_url":"https://api.github.com/users/MicheldeMessieres/orgs","repos_url":"https://api.github.com/users/MicheldeMessieres/repos","events_url":"https://api.github.com/users/MicheldeMessieres/events{/privacy}","received_events_url":"https://api.github.com/users/MicheldeMessieres/received_events","type":"User","site_admin":false}],"milestone":null,"comments":48,"created_at":"2018-03-30T15:48:21Z","updated_at":"2018-05-03T01:27:13Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/trilinos/Trilinos/pulls/2483","html_url":"https://github.com/trilinos/Trilinos/pull/2483","diff_url":"https://github.com/trilinos/Trilinos/pull/2483.diff","patch_url":"https://github.com/trilinos/Trilinos/pull/2483.patch"},"body":"Implements Dependency Inversion and Injection  (DII) system for Belos.\r\n\r\nNote this is a working implementation but is in progress.\r\nThis PR is setup to facilitate discussion of some design elements.\r\n\r\n<!---\r\nBe sure to select `develop` as the `base` branch against which to create this\r\npull request.  Only pull requests against `develop` will undergo Trilinos'\r\nautomated testing.  Pull requests against `master` will be ignored.\r\n-->\r\n\r\n<!---\r\nProvide a general summary of your changes in the Title above.  If this pull\r\nrequest pertains to a particular package in Trilinos, it's worthwhile to start\r\nthe title with \"PackageName:  \".\r\n-->\r\n\r\n<!---\r\nNote that anything between these delimiters is a comment that will not appear\r\nin the pull request description once created.\r\n-->\r\n\r\n<!---\r\nReplace <teamName> below with the appropriate Trilinos package/team name.\r\n-->\r\n@trilinos/belos \r\n\r\n<!---\r\nReviewers:  If you know someone who is knowledgeable about what you are\r\nchanging, or perhaps someone who should be, and you would like them to review\r\nyour changes before they are accepted, select them from the Reviewers drop-down\r\non the right.\r\n-->\r\n\r\n<!---\r\nAssignees:  If you know anyone who should likely handle bringing this pull\r\nrequest to completion, select them from the Assignees drop-down on the right.\r\nIf you have write-access to Trilinos, this should likely be you.\r\n-->\r\n\r\n<!---\r\nLables:  Choose any applicable package names from the Labels drop-down on the\r\nright.  Additionally, choose a label to indicate the type of issue, for\r\ninstance, bug, build, documentation, enhancement, etc.\r\n-->\r\n\r\n## Description\r\n<!--- Describe your changes in detail. -->\r\nSolverFactoryParent has specialized forms of registerSolverFactoryForLib for each of the three libs, belos, belosepetra, and belostpetra. These link to the proper library and automatically register the required solvers via the SolverFactorParent constructor.\r\n\r\nThese tests were currently set to manually register their solvers but how they are handled will probably need further discussion:\r\n              _Belos ValidParameters\r\n               MueLu LevelWrap\r\n               MueLu Maxwell3D\r\n               ROL_adapters_belos_test_vector_BelosInterface_\r\n\r\nFor complex this is working but needs further discussion. I've disabled the exception in the dummy class constructors which don't have complex, and also disabled two unit tests which check for those exceptions. This allows the DII system to register all Tpetra types automatically. But I think we'll\r\nprobably want to turn off automatic registration for complex Tpetra and then manually register those that have complex. Or something similar.\r\n\r\n## Motivation and Context\r\n<!--- Why is this change required?  What problem does it solve? -->\r\nTo reduce build times.\r\n\r\n\r\n## How Has This Been Tested?\r\n<!---\r\nPlease describe in detail how you tested your changes.  Include details of your\r\ntesting environment and the tests you ran to see how your change affects other\r\nareas of the code.  Consider including configure, build, and test log files.\r\n-->\r\nThis build was tested as follow:\r\n\r\n**SEMS, complex off, debug, parallel, for Belos and forward packages (debug parallel)**\r\n- command: ./checkin-test-sems.sh -j4 --local-do-all --configure --allow-no-pull --enable-packages=‘Belos'\r\n- All tests passing\r\n\r\n**SEMS, complex on, debug, parallel, for Belos and forward packages (debug parallel complex)**\r\n- command: ./checkin-test-sems.sh -j4 --local-do-all --configure --allow-no-pull --enable-packages='Belos' --st-extra-builds=MPI_RELEASE_DEBUG_SHARED_PT_COMPLEX --default-builds=''\r\n- All tests passing except these tests which also fail develop:\r\n  \t      _MueLu_ParameterListInterpreterEpetra_MPI_1\r\n  \t      MueLu_ParameterListInterpreterEpetra_MPI_4\r\n  \t      MueLu_ParameterListInterpreterTpetra_MPI_1\r\n  \t      MueLu_ParameterListInterpreterTpetra_MPI_4_\r\n\r\n**linux machine, complex on, release, parallel, gcc 4.9.3, mpich 3.1.4 (attached linux_par.sh)**\r\n- Also tried this shared libs off/on, ETI off/on, complex off/on with same results\r\n- All tests passing except these tests which also fail develop\r\n\t     _MueLu_ParameterListInterpreterTpetra_MPI_1\r\n\t     MueLu_ParameterListInterpreterTpetra_MPI_4_\r\n\r\n**clang machine, complex on, release, parallel mpich 3.1.4 (attached clang_par.sh ) serial (attached clang_ser.sh)**\r\nClang had a few issues on develop branch as well, so unrelated to this branch. Some tests fail intermittently and need to run some more and evaluate full list. Will open a separate issue for that as they happen on develop also. MueLu driver for clang parallel was giving link errors - need to investigate and will open an issue for that since it also happens on develop. I ran clang parallel with MueLu tests disabled due to this.\r\n\r\n**One clang issue is specific to this new DII branch:**\r\nSpecialization in Belos SolverFactoryParent allows the constructor to link to the proper library and auto register the solver managers. Usually we should be able to have those cpp files then do a premain registration similar to how Ifpack2 was setup. However I found having the premain registration was fine for SEMS, linux builds, and most of clang except these two tests:\r\n\t   _Belos_Tpetra_MultipleSolves_MPI_4\r\n           Ifpack2_AdditiveSchwarz_RILUK_MPI_4_ \r\nSerial was fine, but these would seg fault for clang parallel - did not investigate yet. To get a full working build as a first iteration, I have currently disabled premain registration.\r\n\r\n## Screenshots\r\n<!--- Not obligatory, but is there anything pertinent that we should see? -->\r\n\r\n## Checklist\r\n<!---\r\nGo over all the following points, and put an `x` in all the boxes that apply.\r\nIf you are unsure about any of these, please ask&mdash;we are here to help.\r\n-->\r\n- [ ] My commit messages mention the appropriate GitHub issue numbers.\r\n- [x] My code follows the code style of the affected package(s).\r\n- [x] My change requires a change to the documentation.\r\n- [ ] I have updated the documentation accordingly.\r\n- [x] I have read the [code contribution guidelines](../blob/master/CONTRIBUTING.md) for this project.\r\n- [ ] I have added tests to cover my changes.\r\n- [x] All new and existing tests passed.\r\n- [x] No new compiler warnings were introduced.\r\n- [x] These changes break backwards compatibility.  (Potentially)\r\n\r\n## Additional Information\r\n<!--- Anything else we need to know in evaluating this merge request? -->"},"comment":{"url":"https://api.github.com/repos/trilinos/Trilinos/issues/comments/386168905","html_url":"https://github.com/trilinos/Trilinos/pull/2483#issuecomment-386168905","issue_url":"https://api.github.com/repos/trilinos/Trilinos/issues/2483","id":386168905,"user":{"login":"MicheldeMessieres","id":12844480,"avatar_url":"https://avatars2.githubusercontent.com/u/12844480?v=4","gravatar_id":"","url":"https://api.github.com/users/MicheldeMessieres","html_url":"https://github.com/MicheldeMessieres","followers_url":"https://api.github.com/users/MicheldeMessieres/followers","following_url":"https://api.github.com/users/MicheldeMessieres/following{/other_user}","gists_url":"https://api.github.com/users/MicheldeMessieres/gists{/gist_id}","starred_url":"https://api.github.com/users/MicheldeMessieres/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MicheldeMessieres/subscriptions","organizations_url":"https://api.github.com/users/MicheldeMessieres/orgs","repos_url":"https://api.github.com/users/MicheldeMessieres/repos","events_url":"https://api.github.com/users/MicheldeMessieres/events{/privacy}","received_events_url":"https://api.github.com/users/MicheldeMessieres/received_events","type":"User","site_admin":false},"created_at":"2018-05-03T01:27:13Z","updated_at":"2018-05-03T01:27:13Z","author_association":"CONTRIBUTOR","body":"@mhoemmen The default params will be used in setParameters, getValidParameters, and the initializers of the working parameter values so I don't think local placement can work unless we duplicated them. Would it be ok to use the C++11 inline definitions as non-static class members like this:\r\n\r\n    private:\r\n    // Default solver values.\r\n    const MagnitudeType convtol_default_ = 1e-8;\r\n    const int maxIters_default_ = 1000;\r\n    const bool showMaxResNormOnly_default_ = false;\r\n    const int verbosity_default_ = Belos::Errors;\r\n    const int outputStyle_default_ = Belos::General;\r\n    const int outputFreq_default_ = -1;\r\n    const int defQuorum_default_ = 1;\r\n    const std::string resScale_default_ = \"Norm of Initial Residual\";\r\n    const std::string label_default_ = \"Belos\";\r\n    const Teuchos::RCP<std::ostream> outputStream_default_ = Teuchos::rcp(&std::cout,false);\r\n\r\nNot sure yet if there will be any issues with the rcp so possibly would have to change that one. But otherwise I think this would be sufficient to get the pre-main DII registration up and running. I can try this tomorrow if you agree."}}
{"actor":{"display_login":"MicheldeMessieres"}}