Raise constraint errors together + misc. #807

fealho · 2022-05-19T22:26:23Z

Resolve #801.

Note: this PR also fixes several typos and minor fixes.

codecov-commenter · 2022-05-19T23:08:29Z

Codecov Report

Merging #807 (0d14a47) into master (3632d4c) will decrease coverage by 0.06%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   68.05%   67.98%   -0.07%     
==========================================
  Files          38       38              
  Lines        2883     2899      +16     
==========================================
+ Hits         1962     1971       +9     
- Misses        921      928       +7

Impacted Files	Coverage Δ
sdv/metadata/table.py	`75.47% <37.50%> (-1.34%)`	⬇️
sdv/constraints/base.py	`96.13% <81.25%> (-1.46%)`	⬇️
sdv/constraints/errors.py	`100.00% <100.00%> (ø)`
sdv/constraints/tabular.py	`98.59% <100.00%> (+<0.01%)`	⬆️

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 3632d4c...0d14a47. Read the comment docs.

fealho · 2022-05-24T16:02:43Z

sdv/constraints/base.py

@@ -236,6 +262,8 @@ def _validate_constraint_columns(self, table_data):
            table_data (pandas.DataFrame):
                Table data.
        """
+        self._validate_data_on_constraint(table_data)


These two methods: _validate_data_on_constraint and _validate_constraint_columns should be merged in the future, when the fit_columns_model logic gets dropped.

Let's keep these separate for a couple of reasons:

We may end up keeping the columns model

These are actually two different types of validation. One is checking that the data being transformed actually adheres to the constraint. The other is checking if any columns in the constraint are missing and need to be sampled. Let's keep the logic separate. Maybe changing the name of _validate_constraint_columns to _generate_missing_columns would help or something like that

fealho · 2022-05-24T16:03:35Z

sdv/constraints/tabular.py

@@ -19,7 +19,9 @@
      on the other columns of the table.
    * Between: Ensure that the value in one column is always between the values
      of two other columns/scalars.
+    * Rounding: Round a column based on the specified number of digits.


Unrelated to my changes, we just forgot to add these here when creating the constraints.

tests/unit/constraints/test_base.py

fealho · 2022-05-24T16:13:21Z

@amontanez24 @npatki What is the expected behavior of a constraint when handling_strategy=reject_sampling? Right now it doesn't go through validation, so if no rows can be sampled it simply returns and empty dataframe.

npatki · 2022-05-24T16:23:28Z

@amontanez24 @npatki What is the expected behavior of a constraint when handling_strategy=reject_sampling? Right now it doesn't go through validation, so if no rows can be sampled it simply returns and empty dataframe.

The expected behavior is to handle this the same as conditional sampling with reject sampling. See #809. It would be nice to make these changes in tandem with the constraint changes.

fealho · 2022-05-24T17:11:04Z

tests/integration/test_constraints.py

+    gc = GaussianCopula(constraints=constraints)
+
+    err_msg = re.escape(
+        "\nunsupported operand type(s) for -: 'str' and 'str'"


This PR only implements the "pretty print" for the general case where the data doesn't conform with constraint.is_valid(). If some other error shows up, it will just be printed as usual.

sdv/constraints/base.py

amontanez24 · 2022-05-24T19:04:22Z

sdv/constraints/base.py

@@ -236,6 +262,8 @@ def _validate_constraint_columns(self, table_data):
            table_data (pandas.DataFrame):
                Table data.
        """
+        self._validate_data_on_constraint(table_data)


Let's keep these separate for a couple of reasons:

We may end up keeping the columns model

These are actually two different types of validation. One is checking that the data being transformed actually adheres to the constraint. The other is checking if any columns in the constraint are missing and need to be sampled. Let's keep the logic separate. Maybe changing the name of _validate_constraint_columns to _generate_missing_columns would help or something like that

sdv/constraints/base.py

sdv/constraints/tabular.py

sdv/metadata/table.py

…nsformer

amontanez24 · 2022-05-25T00:38:14Z

tests/unit/metadata/test_table.py

@@ -582,93 +581,6 @@ def test__transform_constraints_drops_columns(self):
        }, index=[0, 1, 2])
        assert result.equals(expected_result)

-    def test__validate_data_on_constraints(self):


Can we keep the tests for this method but just move them to the appropriate place

amontanez24

I think this is almost ready, but we should keep the tests for the _validate_on_constraint method

fealho · 2022-05-25T02:53:55Z

I think this is almost ready, but we should keep the tests for the _validate_on_constraint method

@amontanez24 The old tests didn't really match the new code, so I rewrote them a bit.

pvk-developer

LGTM. Just minor question about the Errors or Error

pvk-developer · 2022-05-25T14:33:37Z

sdv/constraints/errors.py

@@ -3,3 +3,7 @@

 class MissingConstraintColumnError(Exception):
    """Error to use when constraint is provided a table with missing columns."""
+
+
+class MultipleConstraintsErrors(Exception):


Should this be Error instaed of Errors ?

I think Errors is clearer, since it is a list of multiple errors

amontanez24

LGTM!

fealho commented May 24, 2022

View reviewed changes

tests/unit/constraints/test_base.py Show resolved Hide resolved

fealho added 8 commits May 24, 2022 09:27

Add documentation that was forgotten

61ec0b9

Fix typo

f66e754

IN PROGRESS

8339d86

Working version without tests

3ebad2c

.

0d92dd8

Remove unnecessary files

a8064fc

Add test case + print correctly

4158206

Move validate in other validate

7f97758

fealho commented May 24, 2022

View reviewed changes

Add validaton when handling_strategy is reject_sampling

6820ece

fealho force-pushed the issue-801-constraints-errors branch from 31f5623 to 6820ece Compare May 24, 2022 17:44

fealho marked this pull request as ready for review May 24, 2022 18:39

fealho requested a review from a team as a code owner May 24, 2022 18:39

fealho requested review from amontanez24 and pvk-developer and removed request for a team May 24, 2022 18:39

amontanez24 requested changes May 24, 2022

View reviewed changes

Move validation out of other validation method + rename _identity_tra…

c61cebe

…nsformer

fealho requested a review from amontanez24 May 24, 2022 21:17

amontanez24 reviewed May 25, 2022

View reviewed changes

Add old validate_data_on_constraints back

0d14a47

fealho requested a review from amontanez24 May 25, 2022 02:52

pvk-developer approved these changes May 25, 2022

View reviewed changes

amontanez24 approved these changes May 26, 2022

View reviewed changes

fealho merged commit 04cfd65 into master May 26, 2022

fealho deleted the issue-801-constraints-errors branch May 26, 2022 04:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise constraint errors together + misc. #807

Raise constraint errors together + misc. #807

fealho commented May 19, 2022

codecov-commenter commented May 19, 2022 •

edited

Loading

fealho May 24, 2022

amontanez24 May 24, 2022

fealho May 24, 2022

fealho commented May 24, 2022

npatki commented May 24, 2022

fealho May 24, 2022

amontanez24 May 24, 2022

amontanez24 May 25, 2022

amontanez24 left a comment

fealho commented May 25, 2022 •

edited

Loading

pvk-developer left a comment

pvk-developer May 25, 2022

fealho May 25, 2022

amontanez24 left a comment

Raise constraint errors together + misc. #807

Raise constraint errors together + misc. #807

Conversation

fealho commented May 19, 2022

codecov-commenter commented May 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fealho commented May 24, 2022

npatki commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontanez24 left a comment

Choose a reason for hiding this comment

fealho commented May 25, 2022 • edited Loading

pvk-developer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontanez24 left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2022 •

edited

Loading

fealho commented May 25, 2022 •

edited

Loading