-
Notifications
You must be signed in to change notification settings - Fork 19
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
Remove need for representative_periods and graph in add_expression_terms_rep_period_constraints #987
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #987 +/- ##
==========================================
+ Coverage 95.37% 95.43% +0.06%
==========================================
Files 29 29
Lines 1124 1140 +16
==========================================
+ Hits 1072 1088 +16
Misses 52 52 ☔ View full report in Codecov by Sentry. |
42af70a
to
073dad7
Compare
…rms_rep_period_constraints Part of #942
ca58367
to
2a22cf7
Compare
Thanks @abelsiqueira, I have a couple of comments that are for my understanding of the changes rather than improvements. The SQL is understandable and readable. The best is that it is fast and uses less memory. |
Move implementation details to the docstring. Add a comment for each step in the corresponding section
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.
Hi @datejada, thanks for the review and comments. Let me know if my replies answer your questions. I will push updates and re-request a review 👍
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.
@abelsiqueira Thanks for the reply to the comments and the changes. I learnt more about SQL and type stability 😄
Remove graph and representative_periods from add_expression_terms_rep_period_constraints.
Refactor the function to use the DuckDB tables instead of the DataFrames.
This now uses DuckDB to compute the groups and the intersection. The type instability made the code a bit slower so to improve, we have to manually specify the types of some outputs with
::Type
. This made the code faster than it was before!Related issues
Part of #942
Checklist
Docs were updated and workflow is passing