-
Notifications
You must be signed in to change notification settings - Fork 556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ifpack2: fix ambiguous MDF unit test #12078
Conversation
@ndellingwood @lucbv this PR (along with kokkos/kokkos-kernels#1916) should address kokkos/kokkos-kernels#1901 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmranse thanks for the work on this! Changes look reasonable, but just to clarify are these independent of kokkos/kokkos-kernels#1916 ? If not, there will need to be some Kokkos version if-guards to preserve the behavior compatible with Trilinos develop branch
@ndellingwood correct neither change should be affected by the version of the other, they're just two issues that contributed to the failure. So I think we're ok without if-guards here. |
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_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: tmranse |
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_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
Failures in the gnu/8.3.0 and clang/11.0.1 builds, here are output snips:
|
@lucbv The root issue happening here is that the old version of this Trilinos test has identical discarded fill in all rows on the first iteration, but in floating points they might be slightly different and our tie breakers aren't applied. This change fixes the test to avoid that issue, but now hits the initialization issue fixed in kokkos/kokkos-kernels#1916 . Alternatively we could replace the checks on if discarded fill are equal with some @ndellingwood The KK change is definitely a bug fix so I'm not sure I want to if-guard back to the old wrong behavior in Trilinos. I also don't want to reconfigure the test to pass by avoiding that bug because I want it covered in the future. I think its perhaps better to just keep this test disabled in Trilinos until the next Kokkos Kernels integration in Trilinos. Do either of you have thoughts? |
@tmranse I think once kokkos/kokkos-kernels#1916 makes it through CI and merges it can be cherry-picked to this PR as an emergency bug-fix to Trilinos (this differs from our usual convention of merging kokkos-kernels changes primarily with release snapshots, but emergency bug fixes can merit this). @lucbv are you okay with this plan, or would you prefer to have the ifpack2 mdf tests disabled in Trilinos until the next release? |
FWIW I just checked and the existing test passes if I change the |
@lucbv any thoughts on these options? looks like you merged kokkos/kokkos-kernels#1916 so maybe cherrypicking is the easiest path forward? |
@tmranse if you don't mind, please go ahead and cherry-pick kokkos/kokkos-kernels#1916 to the PR. I will review after the cherry-pick to ensure no files are clobbered with other bug-fixes merged to Trilinos |
Thanks @ndellingwood and @tmranse for this PR and sorry for the slow response, I created #12140 to cherry pick the changes from Kokkos Kernels. I am hoping this won't take long to merge with Trilinos and we should be able to rebase and retest this one? |
Thanks @lucbv ! |
f8afe73
to
ef5deb7
Compare
@ndellingwood @lucbv looks like #12140 on its own has the same problem of being sensitive to the ambiguous test. cherrypicked here so the changes can go through together |
Thanks @tmranse in that case I will wait to see how testing goes here and we might just close the other PR if that happens. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me
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_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: tmranse |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: tmranse |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
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... |
…s:develop' (cddbf30). * trilinos-develop: (39 commits) KokkosSparse_spmv_bsrmatrix_impl.hpp: const scalars are still scalars Stokhos: Remove random access memory trait in PCE spmv specializations Stokhos: make view array assignment with random-access LHS work Disallow non-scalar types in KokkosKernels BsrMatrix SpMV Tpetra_BlockCrsMatrix_def.hpp: Use KokkosSparse::spmv MueLu: Removing H2D/D2H transfers in CoordinatesTransferFactory (at least for scalar pdes) ifpack2: fix ambiguous MDF unit test (trilinos#12078) Intrepid2: add H(vol) hierarchical pyramids (trilinos#12136) MueLu: Minor cleanups to Kokkos UncoupledAggregation MueLu: Minor cleanups to Kokkos UncoupledAggregation Tpetra: Upgrades to DeepCopyTimer tool added some new comments about lowCommMakeColMap serial vs device Tpetra: Minor fixes to GDSWstyle test Tpetra: Fixing deprecated code path added range policy to parallel loops Reverted execution space changes on views removing extraneous files from lowCommunicationMakeColMap PR changed execution space for Kokkos views, added selection for serial vs CUDA on lowCommMakeColMap explicitly specified the execution space of the ColIndices device view moved colind_LID device to host for case when NumRemoteColGIDs=0 ...
…s:develop' (cddbf30). * trilinos-develop: (39 commits) KokkosSparse_spmv_bsrmatrix_impl.hpp: const scalars are still scalars Stokhos: Remove random access memory trait in PCE spmv specializations Stokhos: make view array assignment with random-access LHS work Disallow non-scalar types in KokkosKernels BsrMatrix SpMV Tpetra_BlockCrsMatrix_def.hpp: Use KokkosSparse::spmv MueLu: Removing H2D/D2H transfers in CoordinatesTransferFactory (at least for scalar pdes) ifpack2: fix ambiguous MDF unit test (trilinos#12078) Intrepid2: add H(vol) hierarchical pyramids (trilinos#12136) MueLu: Minor cleanups to Kokkos UncoupledAggregation MueLu: Minor cleanups to Kokkos UncoupledAggregation Tpetra: Upgrades to DeepCopyTimer tool added some new comments about lowCommMakeColMap serial vs device Tpetra: Minor fixes to GDSWstyle test Tpetra: Fixing deprecated code path added range policy to parallel loops Reverted execution space changes on views removing extraneous files from lowCommunicationMakeColMap PR changed execution space for Kokkos views, added selection for serial vs CUDA on lowCommMakeColMap explicitly specified the execution space of the ColIndices device view moved colind_LID device to host for case when NumRemoteColGIDs=0 ...
…s:develop' (cddbf30). * trilinos-develop: (39 commits) KokkosSparse_spmv_bsrmatrix_impl.hpp: const scalars are still scalars Stokhos: Remove random access memory trait in PCE spmv specializations Stokhos: make view array assignment with random-access LHS work Disallow non-scalar types in KokkosKernels BsrMatrix SpMV Tpetra_BlockCrsMatrix_def.hpp: Use KokkosSparse::spmv MueLu: Removing H2D/D2H transfers in CoordinatesTransferFactory (at least for scalar pdes) ifpack2: fix ambiguous MDF unit test (trilinos#12078) Intrepid2: add H(vol) hierarchical pyramids (trilinos#12136) MueLu: Minor cleanups to Kokkos UncoupledAggregation MueLu: Minor cleanups to Kokkos UncoupledAggregation Tpetra: Upgrades to DeepCopyTimer tool added some new comments about lowCommMakeColMap serial vs device Tpetra: Minor fixes to GDSWstyle test Tpetra: Fixing deprecated code path added range policy to parallel loops Reverted execution space changes on views removing extraneous files from lowCommunicationMakeColMap PR changed execution space for Kokkos views, added selection for serial vs CUDA on lowCommMakeColMap explicitly specified the execution space of the ColIndices device view moved colind_LID device to host for case when NumRemoteColGIDs=0 ...
…s:develop' (cddbf30). * trilinos-develop: (39 commits) KokkosSparse_spmv_bsrmatrix_impl.hpp: const scalars are still scalars Stokhos: Remove random access memory trait in PCE spmv specializations Stokhos: make view array assignment with random-access LHS work Disallow non-scalar types in KokkosKernels BsrMatrix SpMV Tpetra_BlockCrsMatrix_def.hpp: Use KokkosSparse::spmv MueLu: Removing H2D/D2H transfers in CoordinatesTransferFactory (at least for scalar pdes) ifpack2: fix ambiguous MDF unit test (trilinos#12078) Intrepid2: add H(vol) hierarchical pyramids (trilinos#12136) MueLu: Minor cleanups to Kokkos UncoupledAggregation MueLu: Minor cleanups to Kokkos UncoupledAggregation Tpetra: Upgrades to DeepCopyTimer tool added some new comments about lowCommMakeColMap serial vs device Tpetra: Minor fixes to GDSWstyle test Tpetra: Fixing deprecated code path added range policy to parallel loops Reverted execution space changes on views removing extraneous files from lowCommunicationMakeColMap PR changed execution space for Kokkos views, added selection for serial vs CUDA on lowCommMakeColMap explicitly specified the execution space of the ColIndices device view moved colind_LID device to host for case when NumRemoteColGIDs=0 ...
…s:develop' (cddbf30). * trilinos-develop: (39 commits) KokkosSparse_spmv_bsrmatrix_impl.hpp: const scalars are still scalars Stokhos: Remove random access memory trait in PCE spmv specializations Stokhos: make view array assignment with random-access LHS work Disallow non-scalar types in KokkosKernels BsrMatrix SpMV Tpetra_BlockCrsMatrix_def.hpp: Use KokkosSparse::spmv MueLu: Removing H2D/D2H transfers in CoordinatesTransferFactory (at least for scalar pdes) ifpack2: fix ambiguous MDF unit test (trilinos#12078) Intrepid2: add H(vol) hierarchical pyramids (trilinos#12136) MueLu: Minor cleanups to Kokkos UncoupledAggregation MueLu: Minor cleanups to Kokkos UncoupledAggregation Tpetra: Upgrades to DeepCopyTimer tool added some new comments about lowCommMakeColMap serial vs device Tpetra: Minor fixes to GDSWstyle test Tpetra: Fixing deprecated code path added range policy to parallel loops Reverted execution space changes on views removing extraneous files from lowCommunicationMakeColMap PR changed execution space for Kokkos views, added selection for serial vs CUDA on lowCommMakeColMap explicitly specified the execution space of the ColIndices device view moved colind_LID device to host for case when NumRemoteColGIDs=0 ...
* ifpack2: fix ambigous MDF unit test * Kokkos-Kernels: cherry pick of fixes needed for Ifpack2 MDF
Fixes issue with ambiguous MDF unit with multiple valid permutations so that it is not sensitive to locale (order of operations, etc).
@trilinos/ifpack2