Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

synthesizers=None for benchmark_single_table runs default synthesizers #237

Merged
merged 10 commits into from
Jun 1, 2023

Conversation

fealho
Copy link
Member

@fealho fealho commented May 29, 2023

Resolve #233.

@fealho fealho requested a review from a team as a code owner May 29, 2023 12:46
@fealho fealho requested review from amontanez24, frances-h and pvk-developer and removed request for a team and amontanez24 May 29, 2023 12:46
@codecov-commenter
Copy link

codecov-commenter commented May 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +12.82 🎉

Comparison is base (5334aa4) 43.74% compared to head (057ba85) 56.56%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #237       +/-   ##
===========================================
+ Coverage   43.74%   56.56%   +12.82%     
===========================================
  Files          19       19               
  Lines        1159     1172       +13     
===========================================
+ Hits          507      663      +156     
+ Misses        652      509      -143     
Impacted Files Coverage Δ
sdgym/benchmark.py 74.39% <100.00%> (+40.05%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -80,6 +80,7 @@ def _generate_job_args_list(limit_dataset_size, sdv_datasets, additional_dataset
run_id = os.getenv('RUN_ID') or str(uuid.uuid4())[:10]

# Get list of synthesizer objects
synthesizers = DEFAULT_SYNTHESIZERS if synthesizers is None else synthesizers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the issue, I think we actually want to treat None as not running any synthesizers instead of using the defaults.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is right

@fealho fealho requested a review from frances-h May 31, 2023 17:49
Copy link
Member

@pvk-developer pvk-developer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

Base automatically changed from remove-unnecessary-checks to master May 31, 2023 20:05
Copy link
Contributor

@frances-h frances-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating! :shipit:

Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -418,6 +420,29 @@ def _run_jobs(multi_processing_config, job_args_list, show_progress):
return scores


def _empty_dataframe(compute_quality_score, sdmetrics):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: would prefer _get_empty_dataframe

@fealho fealho merged commit aac069c into master Jun 1, 2023
@fealho fealho deleted the issue-233-none-synthesizers branch June 1, 2023 19:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing None as synthesizers runs all of them
5 participants