Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use HierarchicalSampler mixin with HMA #1449

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Use HierarchicalSampler mixin with HMA #1449

merged 2 commits into from
Jun 21, 2023

Conversation

frances-h
Copy link
Contributor

Resolve #1428

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2023 •
edited
Loading

Codecov Report

Patch coverage: 88.46% and project coverage change: +0.13 🎉

Comparison is base (766d7fc) 96.19% compared to head (757ea16) 96.33%.

❗ 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    #1449      +/-   ##
==========================================
+ Coverage   96.19%   96.33%   +0.13%     
==========================================
  Files          49       49              
  Lines        3992     3925      -67     
==========================================
- Hits         3840     3781      -59     
+ Misses        152      144       -8
Impacted Files Coverage Δ
sdv/multi_table/hma.py 80.12% <85.00%> (-2.51%) ⬇️
sdv/sampling/__init__.py 100.00% <100.00%> (ø)
sdv/sampling/hierarchical_sampler.py 100.00% <100.00%> (ø)
sdv/single_table/base.py 99.28% <100.00%> (+<0.01%) ⬆️

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

@frances-h frances-h force-pushed the issue-1428-hma-use-hierarchical-sampler-mixin branch from b254d40 to 6661b9a Compare June 13, 2023 17:48
@frances-h frances-h marked this pull request as ready for review June 14, 2023 17:36
@frances-h frances-h requested a review from a team as a code owner June 14, 2023 17:36
@frances-h frances-h requested review from fealho and pvk-developer and removed request for a team June 14, 2023 17:36
fealho
fealho approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@fealho fealho left a comment

Choose a reason for hiding this comment

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

Looks good to me ⚡

sdv/multi_table/hma.py Show resolved Hide resolved
sdv/sampling/hierarchical_sampler.py
@@ -68,7 +68,7 @@ def _sample_rows(self, synthesizer, num_rows=None):
Sampled rows, shape (, num_rows)
"""
num_rows = num_rows or synthesizer._num_rows
return synthesizer._sample_batch(num_rows, remove_extra_columns=False)
return synthesizer._sample_batch(int(num_rows), keep_extra_columns=True)
Copy link
Member

Choose a reason for hiding this comment

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

@amontanez24 @pvk-developer This line is an example where using an assert could be useful. num_rows should always be an integer, and if it is not then wherever it was set did so incorrectly, so the code there should be updated to cast it to an int.

So at this point we should assert isinstance(num_rows, int), or change the docstring to say that num_rows can be a float as well. You shouldn't have to write code assuming other methods/inputs are invalid, ie casting to int in this case.

sdv/single_table/base.py Outdated Show resolved Hide resolved
amontanez24
amontanez24 reviewed Jun 15, 2023
View reviewed changes
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.

Mostly minor comments! This is looking good :)

sdv/multi_table/hma.py Show resolved Hide resolved
sdv/multi_table/hma.py Show resolved Hide resolved
sdv/single_table/base.py Outdated Show resolved Hide resolved
sdv/single_table/base.py Outdated Show resolved Hide resolved
amontanez24
amontanez24 approved these changes Jun 20, 2023
View reviewed changes
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.

I think this looks good!

@frances-h frances-h force-pushed the issue-1428-hma-use-hierarchical-sampler-mixin branch from 98b7a83 to 757ea16 Compare June 21, 2023 18:15
@frances-h frances-h merged commit 50392fa into master Jun 21, 2023
34 checks passed
@frances-h frances-h deleted the issue-1428-hma-use-hierarchical-sampler-mixin branch June 21, 2023 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amontanez24 amontanez24 amontanez24 approved these changes

@fealho fealho fealho approved these changes

@pvk-developer pvk-developer Awaiting requested review from pvk-developer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make HMA use hierarchical sampling mixin
4 participants
@frances-h @codecov-commenter @amontanez24 @fealho