-
Notifications
You must be signed in to change notification settings - Fork 105
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
Port MPI to Taal #292
Port MPI to Taal #292
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #292 +/- ##
==========================================
+ Coverage 88.44% 88.57% +0.13%
==========================================
Files 87 90 +3
Lines 13460 13765 +305
==========================================
+ Hits 11905 12193 +288
- Misses 1555 1572 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
So, first round of reviews done. Great job in porting all that MPI stuff to Taal! There are still a few issues to iron out, but nothing I see is unsolvable.
If it helps, we should consider setting up a call to talk about some of the larger issues instead of long a back-and-forth via comments. Just let me know...
Co-authored-by: Michael Schlottke-Lakemper <sloede@users.noreply.github.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.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 when all tests pass. Great work!
By the way, have you ever seen one of the MPI tests fail? IIRC, I've never experienced that before, so I cannot say for certain that the overall testing would fail in that case.
Or put differently: Should the tests currently pass, we have a problem with MPI testing (caused by me, since I set it up in the first place), since currently a file is tested that does not even exist:
|
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
As expected. The MPI tests should have failed but they don't. |
Why should they have failed? We get
in https://github.com/trixi-framework/Trixi.jl/pull/292/checks?check_run_id=1390251395. |
Ah, never mind. Didn't see that you fixed the elixir names. |
As discussed, I've tried to use a minimally invasive approach to port MPI to Taal. In most cases, I dispatch on the
mesh
type. In the future, I would like to create an MPI array type foru
etc., which handles gather operations etc. to enable error-based timestep control in DiffEq. However, that's left for future work and can be done once we've settled on our approach to MPI.Closes #287