-
Notifications
You must be signed in to change notification settings - Fork 65
Add working multi table benchmark #504
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
Add working multi table benchmark #504
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature_branch/mutli_table_benchmark #504 +/- ##
========================================================================
+ Coverage 77.04% 77.98% +0.93%
========================================================================
Files 30 30
Lines 2496 2557 +61
========================================================================
+ Hits 1923 1994 +71
+ Misses 573 563 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
df5e11c to
c51dc98
Compare
amontanez24
left a comment
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.
Looking great!
tests/unit/test_benchmark.py
Outdated
| empty_scores = pd.DataFrame() | ||
| mock__generate_job_args_list.return_value = [] | ||
| mock__get_empty_dataframe.return_value = empty_scores |
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.
if you are passing in no synthesizers or datasets, why do you also need to mock the jobs returned? Shouldn't that list be empty?
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.
@pvk-developer any word on this?
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.
Done here: 7dd15e7
The initial idea was to test that functions are properly called but those were already tested anyways in the previous tests so I removed the mocks that were not really needed.
R-Palazzo
left a comment
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.
Looks good!
After addressing @amontanez24's comments, we should be good to go :)
f5abf35 to
cf085f5
Compare
561b1cb to
15288cf
Compare
R-Palazzo
left a comment
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 like the _import_and_validate_synthesizers
e14f7ff
into
feature_branch/mutli_table_benchmark
Resolves #486
CU-86b7cjbz8