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

Add failing test for multiple ranks and threads #16336

Merged
merged 1 commit into from Dec 13, 2023

Conversation

jh66637
Copy link
Contributor

@jh66637 jh66637 commented Dec 9, 2023

@kronbichler @peterrum

MatrixFree::reinit() fails with multiple ranks and threading enabled. I traced this down to the update of values on inner faces. The error reads

An error occurred in line <1005> of file </home/johannes/workspace/dealii/include/deal.II/lac/dynamic_sparsity_pattern.h> in function
    void dealii::DynamicSparsityPattern::add(dealii::DynamicSparsityPattern::size_type, dealii::DynamicSparsityPattern::size_type)
The violated condition was: 
    ::dealii::deal_II_exceptions::internals::compare_less_than(i, n_rows())
Additional information: 
    Index 16 is not in the half-open range [0,16).

Using only one thread (Utilities::MPI::MPI_InitFinalize mpi_init(argc, argv, 1)) works as expected. Running with one rank and threading enabled works as well.

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

This is a good test. I checked #16340 with it. I would suggest to run it only with 2 MPI rank because I think that exposes the bug already. We should try not to use more resources than necessary, because we do MPI + threads, causing a lot of load on the tester.

mpi_initlog();

do_test<2,double>(3,3);
do_test<3,double>(3,3);
Copy link
Member

Choose a reason for hiding this comment

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

Does the 3D test need to be such big? I would hope that 2 refinements and a polynomial degree 2 should suffice (the 2D variant is ok).

@kronbichler
Copy link
Member

You need to run the indent script on this test.

@kronbichler
Copy link
Member

kronbichler commented Dec 11, 2023

I suggest waiting with merging this test until #16340 is merged, to be sure things are fine.

@jh66637
Copy link
Contributor Author

jh66637 commented Dec 11, 2023

This is fine for me. Thank you for the quick fix :)

@kronbichler
Copy link
Member

/rebuild

@kronbichler
Copy link
Member

Could you please rebase this PR to pass the test?

@jh66637 jh66637 force-pushed the mf_reinit_threading_test branch 2 times, most recently from ff982e1 to 83cece4 Compare December 12, 2023 13:29
@masterleinad masterleinad merged commit bd789d7 into dealii:master Dec 13, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants