-
Notifications
You must be signed in to change notification settings - Fork 25
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
Find max value in parallel safely #953
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## fenicsx #953 +/- ##
===========================================
+ Coverage 96.02% 96.14% +0.12%
===========================================
Files 45 46 +1
Lines 2490 2571 +81
===========================================
+ Hits 2391 2472 +81
Misses 99 99 ☔ View full report in Codecov by Sentry. |
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.
I guess we'll need this for maxinum surface, minimum surface, and minimum volume too
src/festim/exports/maximum_volume.py
Outdated
max_value = max(self.field.solution.x.array[indices]) | ||
MPI.COMM_WORLD.barrier() |
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.
- I believe we still want to use numpy.max instead of native max here for performance purposes, no?
- what does
barrier()
do?
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.
-
Yes sorry, I had just copied an example I had found. Now just using numpy within the `allreduce' function instead
-
It seems the barrier function forces all processes to reach the barrier before any of them can continue execution, however for this case it may not be necessary as all the processors will be following the same logical flow. Do you have any thoughts on this @jorgensd ?
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.
Plus shouldn't it be solution.function_space.mesh.comm.barrier()
instead of comm world?
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.
I guess so
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, @jorgensd do you know if the barrier
call is required?
You do not need the barrier as you are using all reduce. a barrier would be necessary if say only one process does something (or a subset), and the others have to wait for them to finish. I almost exclusively use this whenever I create meshes with gmsh, but not very often otherwise. |
Proposed changes
fix for #900
use MPI to find max value
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...