Remove handling_strategy parameter #843
Conversation
Codecov Report
@@ Coverage Diff @@
## master #843 +/- ##
==========================================
+ Coverage 67.69% 69.62% +1.93%
==========================================
Files 38 38
Lines 2857 3009 +152
==========================================
+ Hits 1934 2095 +161
+ Misses 923 914 -9
Continue to review full report at Codecov.
|
4adf5ab to
746e8c0
Compare
4c6f834 to
6539c67
Compare
|
Andrew Montanez seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
sdv/constraints/base.py
Outdated
| self.reverse_transform(transformed) | ||
| return transformed | ||
|
|
||
| except Exception: |
There was a problem hiding this comment.
Can you use the specific errors?
There was a problem hiding this comment.
we want to catch all possible exceptions here
sdv/constraints/base.py
Outdated
| try: | ||
| transformed = self._transform(table_data) | ||
| if self.is_custom: | ||
| self.reverse_transform(transformed) |
There was a problem hiding this comment.
Why is this needed for custom transform?
There was a problem hiding this comment.
custom constraints can have a noop transform but a reverse_transform that requires specific columns, so the transform won't raise any errors but the reverse_transform will.
sdv/constraints/base.py
Outdated
| @@ -182,22 +178,8 @@ def transform(self, table_data): | |||
| pandas.DataFrame: | |||
| Input data unmodified. | |||
| """ | |||
| self._use_reject_sampling = False | |||
| self._validate_all_columns_present(table_data) | |||
There was a problem hiding this comment.
I think we can just copy paste the function logic here, instead of creating it separetely. The docstrings already describe that method either way, and it doesn't really have any test cases.
sdv/metadata/table.py
Outdated
| def _warn_of_missing_columns(constraint, error): | ||
| warnings.warn( | ||
| f'{constraint.__class__.__name__} cannot be transformed because columns: ' | ||
| f'{error.missing_columns} are not found. Using the reject sampling approach ' |
| def check_missing_columns(self, table_data): | ||
| """Check ``table_data`` for missing columns. | ||
| def _fit(self, table_data): | ||
| del table_data |
There was a problem hiding this comment.
I'm wondering if we need this. Couldn't we just use pass or I'm missing something ?
There was a problem hiding this comment.
we could, this is just from before
There was a problem hiding this comment.
LGTM! I left a minor change to the sre_parse I believe it was fine before:
[SDV] ***@lgn:~/Projects/SDV$ isort -df sdv/metadata/utils.py
--- /home/***/Projects/SDV/sdv/metadata/utils.py:before 2022-06-28 14:17:23.330414
+++ /home/***/Projects/SDV/sdv/metadata/utils.py:after 2022-06-28 14:17:24.234020
@@ -1,11 +1,10 @@
"""Tools to generate strings from regular expressions."""
import re
+import sre_parse
import string
import numpy as np
-
-import sre_parse
| @@ -1,11 +1,12 @@ | |||
| """Tools to generate strings from regular expressions.""" | |||
|
|
|||
| import re | |||
| import sre_parse | |||
There was a problem hiding this comment.
Why is this failing ? sre_parse it's from python's library, I don't think we have to remove it from here.
There was a problem hiding this comment.
It fails on windows and mac
resolves #833