Try to improve performance of contingency_similarity #625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rwedge @pvk-developer I included all the variants I tried. Ultimately we'll just pick one before merging, but I wanted you to see them and give any feedback. Personally I like the option that only uses groupby and not unstack since it seems simplest. |
|
here are the .prof files if you want to examine yourself |
Comment on lines
102
to
113
| elif method == 'groupby_reindex': | ||
| columns = real_data.columns[:2] | ||
| real = real_data[columns] | ||
| synthetic = synthetic_data[columns] | ||
| contingency_real = real.groupby(list(columns), dropna=False).size() / len(real) | ||
| contingency_synthetic = synthetic.groupby(list(columns), dropna=False).size() / len(synthetic) | ||
| combined_index = contingency_real.index.union(contingency_synthetic.index) | ||
| contingency_synthetic = contingency_synthetic.reindex(combined_index, fill_value=0) | ||
| contingency_real = contingency_real.reindex(combined_index, fill_value=0) | ||
| diff = abs(contingency_real - contingency_synthetic).fillna(0) | ||
| variation = diff / 2 | ||
| return 1 - variation.sum() |
There was a problem hiding this comment.
I vote for this approach.
There was a problem hiding this comment.
This seems reasonable to me as well
amontanez24
commented
Aug 27, 2024
| }) | ||
| assert score == 0.6615076057048345 | ||
| assert score == 0.6615076057048344 |
There was a problem hiding this comment.
The new method seems to round the last digit down instead of up
pvk-developer
approved these changes
Aug 28, 2024
rwedge
approved these changes
Aug 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #622
Below is the profiling of the
QualityReportbefore the changes. As you can see most time was spent in thecrosstabmethod.To improve this, I switched to using
groupbyinstead.Groupbyseems to be faster thancrosstabandpivot_tableaccording to this post and trials I ran. The one issue is thatgroupbyreturns data as a multi-indexed series instead of a table with the different column values on one axis and the different row values on the other. To compute the metric, I could either useunstackto convert the return value ofgroupbyto the table we wanted, or change the logic to sum over the multi-indexed series. I tried both examples and show their results below. I used the same dataset for each method.Results for all methods at 1000 rows
Results for all methods at 10,000 rows
Results for all methods at 1,000,000 rows
As you can see, as the number of rows increases, the gap in time between methods decreases. Despite that, groupby was always faster. The specific method of groupby that was fastest seemed to vary based on number of rows.
Below is the profiling for each method.
Using groupby, unstack and reindexing after unstacking
Using groupby, unsstack and reindexing before unstacking
Using groupby on its own