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

Independent Sampler always create a sample size of 1 no matter how small the scale #2102

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Independent Sampler always create a sample size of 1 no matter how small the scale #2102

merged 1 commit into from
Jul 3, 2024

Conversation

lajohn4747
Copy link
Contributor

@lajohn4747 lajohn4747 commented Jul 1, 2024 •
edited
Loading

CU-86b0regxw
resolves issue

Prevents the too small of a sample from erroring out. Similar to: #2045

@lajohn4747 lajohn4747 self-assigned this Jul 1, 2024
@lajohn4747 lajohn4747 requested a review from a team as a code owner July 1, 2024 22:12
@sdv-team
Copy link
Contributor

sdv-team commented Jul 1, 2024

Task linked: CU-86b0regxw SDV-Enterprise - HSA Synthesizer's scale parameter doesn't work for small values #607

amontanez24
amontanez24 reviewed Jul 2, 2024
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.

Looks good! Can we just add an integration test that basically copies the code for recreation in the issue?

@sdv-dev sdv-dev deleted a comment from lajohn4747 Jul 2, 2024
fealho
fealho approved these changes Jul 3, 2024
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 after adding an integration test.

tests/unit/sampling/test_independent_sampler.py
@@ -404,3 +405,36 @@ def _connect_tables(sampled_data):
'transactions': DataFrameMatcher(connected_transactions),
})
assert result == instance._finalize.return_value

def test__sample_too_small(self):
Copy link
Member

Choose a reason for hiding this comment

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

Docstring

@lajohn4747 lajohn4747 merged commit 6a57b3f into main Jul 3, 2024
76 checks passed
@lajohn4747 lajohn4747 deleted the independent_sample_too_small branch July 3, 2024 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fealho fealho fealho approved these changes

@gsheni gsheni gsheni approved these changes

@amontanez24 amontanez24 amontanez24 approved these changes

Assignees

@lajohn4747 lajohn4747

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lajohn4747 @sdv-team @amontanez24 @gsheni @fealho