-
Notifications
You must be signed in to change notification settings - Fork 109
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
Implement AMR with MPI #361
Conversation
Coarsen is only guaranteed to work once. To coarsen again, the tree needs to be partitioned again.
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.
There's one small change (comment to docstring). However, I cannot find a test for the new functionality, or am I missing something? If we haven't talked about it yet - I think it would be good if you can add one test to the 2D parallel tests that exercises this parallel AMR functionality such that we know it works and that it cannot be easily broken by future commits.
Taal's new modularity allows us to create new AMR indicators only for testing purposes without the need to write code into Trixi that will only be used for testing. |
Hm. My thinking is that these tests will become obsolete at some point (i.e., when we have proper parallel AMR tests), so they will not be there permanently. OTOH, storing them in a different location increases complexity and I'll certainly have forgotten about them by the time I have to change all elixirs, and then they get left out. Thus, I'd prefer to have them in their usual location for now - but you should add a comment at the very top of the elixirs (and also next to the test themselves) that these elixirs and indicators do not make much practical sense and are only used for testing. Then it should be clear to everyone what this is all about. |
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.
Thanks for working on this, @erik-f - nice work! I didn't check all algorithms in detail since I assume @sloede had a closer look at everything.
Thus, I'd prefer to have them in their usual location for now - but you should add a comment at the very top of the elixirs (and also next to the test themselves) that these elixirs and indicators do not make much practical sense and are only used for testing. Then it should be clear to everyone what this is all about.
I think this would be good 👍
We have some depwarns:
Warning: `Allgatherv(sendbuf, counts::Vector{Cint}, comm::Comm)` is deprecated, use `Allgatherv!(sendbuf, VBuffer(similar(sendbuf, sum(counts)), counts), comm)` instead.
Warning: `Gatherv(sendbuf::AbstractArray, counts::Vector{Cint}, root::Integer, comm::Comm)` is deprecated, use `Gatherv!(view(sendbuf, 1:counts[MPI.Comm_rank(comm) + 1]), if Comm_rank(comm) == root
│ VBuffer(similar(sendbuf, sum(counts)), counts)
│ else
│ nothing
│ end, root, comm)` instead.
Why does
It doesn't show warnings anymore in the logs but still fails. The other tests however show warnings in the logs, but they don't fail. |
There are still some depwarns related to MPI. Could you please try to fix them? If that doesn't help, could you please run the specific tests that fails locally with |
Otherwise Julia will throw a warning if both examples are run together in the same session.
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.
This looks very, very good. Two minor issues to resolve and then we can merge asap.
Add assertion to make sure that every rank has at least one element even when allow_coarsening is set to true.
I didn't feel 100% confident with the new |
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
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.
LGTM and can be merged once all tests pass & coverage is OK.
Those who merge should also kick CompatHelper to create a compat entry for SimpleMock.jl
in test/Project.toml
.
This could also be added manually (setting the lower version bound to the recent version of SimpleMock.jl). Otherwise, CompatHelper should be scheduled tonight. |
Implement WP3 of #159.
Resolve #330.