-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactoring LightBulb_functions #196
Conversation
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.
Mariam, thank you for taking the lead on making these changes to the lightbulb algorithm. I looked over each of the files, and they look good to me. But then again, I was mostly looking over them to learn. Rather than approving merging, I'll just comment. I'll let Jonah give the final approval for merge.
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.
Overall looks good to me. Two minor changes but otherwise this is good to go.
src/phoebus_driver.cpp
Outdated
const int num_partitions = pmesh->DefaultNumPartitions(); | ||
TaskRegion &sync_region_lb = tc.AddRegion(num_partitions); | ||
for (int ib = 0; ib < num_partitions; ++ib) { | ||
auto &base = pmesh->mesh_data.GetOrAdd("base", ib); |
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.
Probably don't need to pull out base
explicitly. Just use sc0
.
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.
removed this line
Co-authored-by: Jonah Miller <jonah.maxwell.miller@gmail.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.
Assuming tests pass (and it runs for you) we're good to merge.
PR Summary
PR Checklist
scripts/bash/format.sh
.