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

Refactor sample API #705

Merged
merged 12 commits into from
Feb 23, 2022
Merged

Refactor sample API #705

merged 12 commits into from
Feb 23, 2022

Conversation

katxiao
Copy link
Contributor

@katxiao katxiao commented Feb 9, 2022 •
edited
Loading

Resolves #690

@katxiao katxiao force-pushed the issue-690-improve-sample branch from 1ee38c2 to 6ba338d Compare February 9, 2022 01:14
@katxiao katxiao force-pushed the issue-689-add-conditions-class branch from 2749c29 to 247dce7 Compare February 10, 2022 00:29
@katxiao katxiao force-pushed the issue-690-improve-sample branch from 6ba338d to 45fd128 Compare February 10, 2022 00:37
@katxiao katxiao changed the title Issue 690 improve sample Refactor sample API Feb 10, 2022
@katxiao katxiao marked this pull request as ready for review February 10, 2022 00:49
@katxiao katxiao requested a review from a team as a code owner February 10, 2022 00:49
@katxiao katxiao requested review from fealho and removed request for a team February 10, 2022 00:49
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2022 •
edited
Loading

Codecov Report

Merging #705 (318ffbc) into v0.14.0.dev (f680e9d) will increase coverage by 0.62%.
The diff coverage is 90.78%.

Impacted file tree graph

@@               Coverage Diff               @@
##           v0.14.0.dev     #705      +/-   ##
===============================================
+ Coverage        65.23%   65.86%   +0.62%     
===============================================
  Files               36       36              
  Lines             2606     2651      +45     
===============================================
+ Hits              1700     1746      +46     
+ Misses             906      905       -1
Impacted Files Coverage Δ
sdv/relational/hma.py 11.48% <0.00%> (-0.05%) ⬇️
sdv/tabular/base.py 79.09% <92.00%> (+6.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f680e9d...318ffbc. Read the comment docs.

@katxiao katxiao force-pushed the issue-689-add-conditions-class branch from 247dce7 to 22bd94e Compare February 10, 2022 20:15
@katxiao katxiao force-pushed the issue-690-improve-sample branch 2 times, most recently from 835890e to a2c2487 Compare February 10, 2022 22:11
Base automatically changed from issue-689-add-conditions-class to v0.14.0.dev February 11, 2022 01:01
@katxiao katxiao force-pushed the issue-690-improve-sample branch 2 times, most recently from b222a86 to c38c0b4 Compare February 11, 2022 01:44
@katxiao katxiao changed the title Refactor sample API Refactor sample API (1/3) Feb 11, 2022
@katxiao katxiao requested a review from csala February 11, 2022 18:08
fealho
fealho reviewed Feb 14, 2022
View reviewed changes
fealho
fealho reviewed Feb 14, 2022
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 so far. I'm not sure if I should wait for the other PR's to be merged before looking at it again?

tests/integration/timeseries/test_par.py
@@ -62,7 +62,7 @@ def test_column_after_date_simple():

model = PAR(entity_columns=['col'], sequence_index='date', epochs=1)
model.fit(data)
sampled = model.sample()
sampled = model.sample(1)
Copy link
Member

Choose a reason for hiding this comment

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

Why does this work? Does one sample from PAR generate 2 rows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it has something to do with the number of sequences, and how PAR generates sequences after the usual sample process.

fealho
fealho reviewed Feb 14, 2022
View reviewed changes
@katxiao katxiao requested a review from fealho February 16, 2022 21:58
@katxiao katxiao force-pushed the issue-690-improve-sample branch from 5cc63b3 to a50c40f Compare February 16, 2022 22:13
@katxiao katxiao force-pushed the issue-690-improve-sample branch from a50c40f to 4c79e0b Compare February 16, 2022 23:57
@katxiao katxiao force-pushed the issue-690-improve-sample branch from 4c79e0b to e3bf2a9 Compare February 17, 2022 19:21
csala
csala suggested changes Feb 18, 2022
View reviewed changes
@katxiao katxiao force-pushed the issue-690-improve-sample branch from e3bf2a9 to bdec209 Compare February 22, 2022 19:01
@katxiao
Add sample conditions method
a919d02
@katxiao
add back unit tests with conditions
c16afd0
@katxiao
Update logic for handling multiple conditions
fcf23ba
@katxiao
fix lint
74271ea
@katxiao
fix integration tests
c8684c7
@katxiao
Add method to sample remaining columns (3/3) (#708)
318ffbc 
* Add method to sample remaining columns

* update integration tests

* add unit tests

* update tutorials and docs

* Enable batch sampling (#709)

* Add batch sampling and progress bar

* Make sure to close progress bar

* Periodically write to file

* add unit tests

* cr comments

* fix test
@katxiao katxiao merged commit 0dae889 into v0.14.0.dev Feb 23, 2022
@katxiao katxiao deleted the issue-690-improve-sample branch February 23, 2022 05:19
@katxiao katxiao changed the title Refactor sample API (1/3) Refactor sample API Feb 23, 2022
katxiao added a commit that referenced this pull request Mar 3, 2022
@katxiao
Refactor sample API (1/3) (#705)
b2b817d 
* Update sample method args

* Add unit tests

* remove conditioning logic for now

* Add error handling

* Make integration tests pass

* code review comments

* Add sample conditions method

* add back unit tests with conditions

* Update logic for handling multiple conditions

* fix lint

* fix integration tests

* Add method to sample remaining columns (3/3) (#708)

* Add method to sample remaining columns

* update integration tests

* add unit tests

* update tutorials and docs

* Enable batch sampling (#709)

* Add batch sampling and progress bar

* Make sure to close progress bar

* Periodically write to file

* add unit tests

* cr comments

* fix test
katxiao added a commit that referenced this pull request Mar 4, 2022
@katxiao
Refactor sample API (1/3) (#705)
2018d9e 
* Update sample method args

* Add unit tests

* remove conditioning logic for now

* Add error handling

* Make integration tests pass

* code review comments

* Add sample conditions method

* add back unit tests with conditions

* Update logic for handling multiple conditions

* fix lint

* fix integration tests

* Add method to sample remaining columns (3/3) (#708)

* Add method to sample remaining columns

* update integration tests

* add unit tests

* update tutorials and docs

* Enable batch sampling (#709)

* Add batch sampling and progress bar

* Make sure to close progress bar

* Periodically write to file

* add unit tests

* cr comments

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csala csala csala approved these changes

@fealho fealho fealho approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@katxiao @codecov-commenter @csala @fealho