Skip to content
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

[Core][MPI] Update test suite for DistributedVectorExporter #11173

Merged
merged 8 commits into from
Sep 20, 2023

Conversation

loumalouomega
Copy link
Member

📝 Description

This PR updates the test suite for the DistributedVectorExporter in the file test_distributed_exporter.cpp. The change modifies the test case's suite from KratosCoreFastSuite to KratosMPICoreFastSuite.

NOTE: The test is failing, like the suite was wrong it was not running in the CI, so we lost track when stopped working, @RiccardoRossi took a look

🆕 Changelog

@loumalouomega loumalouomega added Kratos Core Testing Parallel-MPI Distributed memory parallelism for HPC / clusters FastPR This Pr is simple and / or has been already tested and the revision should be fast labels May 23, 2023
@loumalouomega
Copy link
Member Author

You can see the false positives @jcotela : https://github.com/KratosMultiphysics/Kratos/actions/runs/5055290302/jobs/9071307435

 |  /           |                  
 ' /   __| _` | __|  _ \   __|    
 . \  |   (   | |   (   |\__ \  
_|\_\_|  \__,_|\__|\___/ ____/
           Multi-Physics 9.3."1"--2eaf8a4-Custom-x86_64
           Compiled for GNU/Linux and Python3.10 with Clang-14.0
Compiled with threading and MPI support.
Maximum number of threads: 1.
MPI world size:         2.
Process Id: 3883 
TestAssignMPIUniqueModelPartCollectionTagUtility                        OK.
TestBenchmarkDistributedGraphConstructionMPI                            OK.
...blah blah
TestDistributedVectorExporter                                           FAILED!
...blah blah
Ran 236 of 1544 test cases in 0.191161s (2 skipped). 1 failed:
    TestDistributedVectorExporter Failed with messages: 
From rank 0:
Test was reported as successful on this rank, but failed on a different rank.
From rank 1:
Error: Check failed because x[x.GetNumbering().LocalId(total_size-1)] = 20 is not near to 15.0 = 15 within the tolerance 1e-14
in kratos/mpi/tests/cpp_tests/sources/test_distributed_exporter.cpp:60: virtual void Testing::TestDistributedVectorExporter::TestFunction()

@loumalouomega
Copy link
Member Author

After the merge with master should break

@loumalouomega
Copy link
Member Author

@RiccardoRossi you can see that with more than 2 partitions the test fails

@loumalouomega
Copy link
Member Author

@RiccardoRossi check this

@loumalouomega
Copy link
Member Author

Reping @RiccardoRossi

@RiccardoRossi
Copy link
Member

i have no time to check this now... can u try to debug what happens here?

@loumalouomega
Copy link
Member Author

i have no time to check this now... can u try to debug what happens here?

I already tried and I could find what was the problem. I don't have the time neither for more debug.

@@ -51,10 +51,10 @@ KRATOS_DISTRIBUTED_TEST_CASE_IN_SUITE(DistributedVectorExporter, KratosMPICoreFa
if(x.GetNumbering().IsLocal(0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a quick look into this, and my impression is that x is not initialized ... so it addes the 5,9,15 to a vector that may contain anything...

@RiccardoRossi
Copy link
Member

@loumalouomega it looks like this fixes it (pending all the tests are finished)

@RiccardoRossi RiccardoRossi marked this pull request as ready for review September 20, 2023 13:58
@RiccardoRossi RiccardoRossi requested review from a team as code owners September 20, 2023 13:58
@RiccardoRossi
Copy link
Member

@loumalouomega you opened this, so I can approve it. Feel free to merge it when you like

@loumalouomega loumalouomega merged commit cefc327 into master Sep 20, 2023
@loumalouomega loumalouomega deleted the core/mpi/correct-wrong-suite-test branch September 20, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FastPR This Pr is simple and / or has been already tested and the revision should be fast Kratos Core Parallel-MPI Distributed memory parallelism for HPC / clusters Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants