-
Notifications
You must be signed in to change notification settings - Fork 9
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
Speedup thicket composition by performing profile unions in binary tree order #170
Speedup thicket composition by performing profile unions in binary tree order #170
Conversation
00e02ba
to
e4d2ebc
Compare
e4d2ebc
to
f57450e
Compare
f57450e
to
fe69690
Compare
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.
The code is good, but something has to be done to make the tree-based concatenation clearer. At bare minimum, this needs to be well commented, otherwise no one is going to have a clue what this is doing.
thicket/thicket.py
Outdated
|
||
return thicket_object | ||
# n - 1 edges in a binary tree | ||
pbar = tqdm.tqdm(range(len(ens_list) - 1), disable=disable_tqdm) |
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.
Using tqdm
here makes this process very hard to understand. I'd suggest not using tqdm
and/or extensively documenting this with comments.
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.
tqdm
is really useful to see what is happening for longer reads.
I changed it to use a while loop, which should be clearer with the popping and appending that is happening in the loop. And I added very detailed comments.
fe69690
to
bce8513
Compare
bce8513
to
bb1b19d
Compare
6fd50d7
to
a0afa2d
Compare
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. The stuff you're doing with tqdm also makes much more sense now that you've added the comments and switched to a while
loop
@pearce8 this PR is also ready for your review |
…ee order (LLNL#170) * Speedup reader by splitting into chunks * Switch to binary tree order composition * Make code clearer with while loop and comments * Clarity * Update comment * Update comment --------- Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1154.llnl.gov>
…ee order (LLNL#170) * Speedup reader by splitting into chunks * Switch to binary tree order composition * Make code clearer with while loop and comments * Clarity * Update comment * Update comment --------- Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1154.llnl.gov>
…ee order (LLNL#170) * Speedup reader by splitting into chunks * Switch to binary tree order composition * Make code clearer with while loop and comments * Clarity * Update comment * Update comment --------- Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1154.llnl.gov>
This PR extends off #169. Limiting the total size of the ensemble during unification is more impactful than the performance gains from parallelism, and this solution is less complicated than using
multiprocessing
.Number of unions is not affected by the different unifying strategy in this PR. This is because:
This PR increases performance by keeping the
old_to_new
mapping dictionary small as we callconcat_thickets
n-1
times instead of one time (as in the develop branch). Theold_to_new
dictionary requires an operation to update the dictionary mappings per each union, which can become expensive if the dictionary is too large. Theold_to_new
dictionary is necessary to replace the node objects in the performance data with the new node objects in the graph that results fromunion
.