Add Inequality CAG #2405
Add Inequality CAG #2405
Conversation
|
Task linked: CU-86b42406t SDV - Add |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/single-table-CAG #2405 +/- ##
============================================================
+ Coverage 98.55% 98.56% +0.01%
============================================================
Files 65 66 +1
Lines 6425 6563 +138
============================================================
+ Hits 6332 6469 +137
- Misses 93 94 +1
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:
|
fealho
force-pushed
the
issue-2384-inquality-cags
branch
from
74151e2 to
2cc3cc4
Compare
There was a problem hiding this comment.
This is looking really good!
I let some suggestions about the previous code definition but mainly can we ensure that any parameter setting is done during fit() instead of transform().
I also have one question regarding the single-table check in validate(), maybe something in the base should be updated.
sdv/cag/base.py
Outdated
| if isinstance(data, pd.DataFrame): | ||
| if self._single_table: | ||
| data = {self._table_name: data} | ||
| else: | ||
| table_name = self._get_single_table_name(metadata) | ||
| data = {table_name: data} |
There was a problem hiding this comment.
@amontanez24 do you remember if we should be able to validate a cag before calling fit()?
If this is the case we may want to move the following logic outside of fit() so it can be used here as well:
https://github.com/sdv-dev/SDV/blob/e96754ed3f1d892f7842898703ceb14369a846df/sdv/cag/base.py#L77-80
Also could we only keep if isinstance(data, pd.DataFrame): and remove if data is not None: before.
There was a problem hiding this comment.
I don't think validate should set any attributes, it should simply check the given parameters are valid.
| if nan_col is not None: | ||
| self._nan_column_name = _create_unique_name(nan_col.name, table_data.columns) |
There was a problem hiding this comment.
Could we also move this in fit()?
There was a problem hiding this comment.
As was discussed here, this PR only converts the existing constraint to a CAG. I agree this is a good suggestion but I don't think it should be implemented in this PR.
| else: | ||
| diff_column = high - low | ||
|
|
||
| self._diff_column_name = _create_unique_name(self._diff_column_name, table_data.columns) |
There was a problem hiding this comment.
Could we move this in fit()?
fealho
force-pushed
the
issue-2384-inquality-cags
branch
from
fd4882b to
f9d68b7
Compare
CU-86b42406t, Resolve #2384.