-
Notifications
You must be signed in to change notification settings - Fork 355
Add CAG validation to synthesizer.validate #2480
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/single-table-CAG #2480 +/- ##
=============================================================
- Coverage 98.55% 83.77% -14.78%
=============================================================
Files 68 68
Lines 7045 7051 +6
=============================================================
- Hits 6943 5907 -1036
- Misses 102 1144 +1042
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:
|
202aa7b
to
d955a9e
Compare
12ec5bc
to
c2c3060
Compare
c2c3060
to
258e10a
Compare
b62ec68
to
14d9936
Compare
14d9936
to
6e7813e
Compare
74f045e
to
85b506c
Compare
sdv/single_table/base.py
Outdated
@@ -775,31 +768,51 @@ def _transform_helper(self, data): | |||
|
|||
return data | |||
|
|||
def preprocess(self, data): | |||
"""Transform the raw data to numerical space. | |||
def _validate_cags(self, data): |
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.
Should this function just call the user-facing validate_cag
function (made for synthetic data)?
Line 719 in 85b506c
def validate_cag(self, synthetic_data): |
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.
On second thought, these two functions seem slightly different.
9eba42f
to
7347cc6
Compare
157d027
to
61cb10e
Compare
61cb10e
to
c5b3e6f
Compare
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 great! Just one question
@@ -133,6 +134,7 @@ def __init__(self, metadata, locales=['en_US'], synthesizer_kwargs=None): | |||
|
|||
self._initialize_models() | |||
self._fitted = False | |||
self._constraints_fitted = False |
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.
this parameter is not used at all for the sampling process right? I'm wondering if we need to worry about the backwards compatibility of 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.
That's a good point. It's not currently used for sampling and I checked that all the tests are passing on enterprise when sdv points to this branch
sdv/multi_table/base.py
Outdated
@@ -233,27 +228,29 @@ def get_metadata(self, version='original'): | |||
|
|||
return Metadata.load_from_dict(self.metadata.to_dict()) | |||
|
|||
def _transform_helper(self, data): | |||
def _validate_transform_constraints(self, data): |
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 re-fitting a CAG is a possibility I would like to suggest a force_fit
argument; Then we would call it always on the validate
but not when we preprocess.
Here is why:
If I run synthesizer.fit(data)
then I would like to 're-fit' because I realized I fitted with my data
but not real_data
, the constraints would have already been fitted with the data
and won't be re-fitted ever.
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.
That's a very good point, thanks! Done in 5b86f37
CU-86b4pmjph
Resolve #2470