Add `validate` to `SingleTableMetadata` #930

fealho · 2022-07-29T16:42:28Z

Resolve #879.

sdv/metadata/errors.py

sdv/metadata/single_table.py

fealho · 2022-08-02T16:53:50Z

sdv/metadata/single_table.py


    @staticmethod
    def _validate_datetime(column_name, **kwargs):
        datetime_format = kwargs.get('datetime_format')
-        if datetime_format:
+        if datetime_format is not None:


Unrelated. It was not validating if the value was an empty string.

sdv/metadata/single_table.py

tests/unit/metadata/test_single_table.py

fealho · 2022-08-02T17:00:34Z

The #879 issue asks us to test Real World (Semantic) Types (ie. phone_number, but this was not done in the validate_columns PR. Do we still want to add this?

codecov-commenter · 2022-08-03T03:48:53Z

Codecov Report

Merging #930 (af34779) into V1.0.0.dev (2fde2d0) will increase coverage by 0.43%.
The diff coverage is 96.55%.

@@              Coverage Diff               @@
##           V1.0.0.dev     #930      +/-   ##
==============================================
+ Coverage       72.73%   73.17%   +0.43%     
==============================================
  Files              40       40              
  Lines            3323     3377      +54     
==============================================
+ Hits             2417     2471      +54     
  Misses            906      906

Impacted Files	Coverage Δ
sdv/metadata/dataset.py	`64.78% <25.00%> (ø)`
sdv/metadata/table.py	`80.99% <50.00%> (ø)`
sdv/constraints/base.py	`97.16% <100.00%> (+0.02%)`	⬆️
sdv/constraints/tabular.py	`99.00% <100.00%> (ø)`
sdv/metadata/__init__.py	`100.00% <100.00%> (ø)`
sdv/metadata/errors.py	`100.00% <100.00%> (ø)`
sdv/metadata/multi_table.py	`98.52% <100.00%> (ø)`
sdv/metadata/single_table.py	`99.14% <100.00%> (+0.24%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

fealho

Left a few comments to help understand the code. Messages marked with Unrelated mean they are standalone and unrelated to the PR itself.

sdv/constraints/base.py

fealho · 2022-08-03T16:15:36Z

sdv/constraints/base.py

@@ -135,16 +136,16 @@ def _validate_metadata_columns(cls, metadata, **kwargs):
        else:
            column_names = kwargs.get('column_names')

-        missing_columns = set(column_names) - set(metadata._columns)
-
+        missing_columns = set(column_names) - set(metadata._columns) - {None}


Unrelated. This happens when a column name was not passed/is missing.

fealho · 2022-08-03T16:16:20Z

sdv/constraints/base.py

            raise ConstraintMetadataError(
-                f'A {cls.__name__} constraint is being applied to invalid column names '
+                f'{article} {cls.__name__} constraint is being applied to invalid column names '


sdv/constraints/base.py

fealho · 2022-08-03T16:23:27Z

sdv/metadata/single_table.py

@@ -250,55 +248,90 @@ def _validate_datatype(id):
        """Check whether id is a string or a tuple of strings."""
        return isinstance(id, str) or isinstance(id, tuple) and all(isinstance(i, str) for i in id)

+    def _validate_key(self, id, key_type):


The previous implementation had the validation inside the set_key methods. Pulling it out of the key methods so I can use it in the validate method.

fealho · 2022-08-03T16:25:43Z

sdv/metadata/single_table.py


-        self._metadata['sequence_index'] = column_name
+    def _validate_constraint(self, constraint_name, **kwargs):


Pulling this out of add_constraint because I need it in for validate.

fealho · 2022-08-03T16:26:49Z

sdv/metadata/single_table.py

+            try:
+                self._validate_constraint(constraint_name, **kwargs)
+            except MultipleConstraintsErrors as e:
+                reformated_errors = '\n'.join(map(str, e.errors))


Need to do this because MultipleConstraintsErrors sets it's own __str__, which messes up the formatting of the error message.

fealho · 2022-08-03T16:28:03Z

sdv/metadata/single_table.py


    def to_dict(self):
        """Return a python ``dict`` representation of the ``SingleTableMetadata``."""
        metadata = {}
-        for key, value in self._metadata.items():
+        for key in self._KEYS:


Change needed because we deleted _metadata.

fealho · 2022-08-03T16:32:49Z

tests/unit/constraints/test_tabular.py

@@ -990,7 +990,7 @@ def test__validate_metadata_specific_to_constraint_datetime_error(self):

        # Run
        error_message = re.escape(
-            'An Inequality constraint is being applied to mismatched sdtypes '
+            'An Inequality constraint is being applied to mismatched sdtype columns '


Unrelated. Phrasing was confusing, it made it seem the values were sdtypes when they are actually columns.

minor: can we reword to is being applied to columns with mismatched sdtypes then? I think that makes more sense

amontanez24

Looking good. A few minor comments

sdv/constraints/base.py

amontanez24 · 2022-08-03T17:28:48Z

tests/unit/constraints/test_tabular.py

@@ -990,7 +990,7 @@ def test__validate_metadata_specific_to_constraint_datetime_error(self):

        # Run
        error_message = re.escape(
-            'An Inequality constraint is being applied to mismatched sdtypes '
+            'An Inequality constraint is being applied to mismatched sdtype columns '


minor: can we reword to is being applied to columns with mismatched sdtypes then? I think that makes more sense

tests/unit/constraints/test_tabular.py

tests/unit/metadata/test_single_table.py

fealho · 2022-08-03T17:48:14Z

@amontanez24 I'm not sure what you mean here: #930 (comment), it was already changed in the base.

amontanez24 · 2022-08-03T17:51:11Z

@amontanez24 I'm not sure what you mean here: #930 (comment), it was already changed in the base.

you're right, I read something wrong

amontanez24

Some very minor comments, but I think this is good to go!

tests/unit/constraints/test_base.py

sdv/metadata/single_table.py

amontanez24 · 2022-08-03T20:09:14Z

sdv/metadata/single_table.py

+            self._append_error(errors, self._validate_column, column, **kwargs)
+
+        if errors:
+            raise InvalidMetadataError(


Not necessary for this PR but later I think we might want to make a custom error that takes a list of errors and joins them in the message. We're probably going to be doing that a lot in the future

…ised)

… be list of tuple

* Add key validation methods (the methods themselves still gotta be revised) * Combined errors implementation * Finish constraints * Add silly constraint solution * Add try/catches for validation + add unit and integration tests * Change MetadataError to InvalidMetadataError * Simplify tests to one element + fix lint * Remove _metadata * Change way _metadata implemented in to_dict * Add constraint validation + integration test + change _constraints to be list of tuple * Fix error message for validate + add error integration test * Fix all constraint related error messages * Add helper _append_error to validate + fix constraint related errors * Add constraint unit test * Fix lint * change _metadata[pk] to pk * Add tests requested + addresses feedback * Address minor feedback

fealho changed the base branch from master to V1.0.0.dev July 29, 2022 16:42

fealho commented Aug 2, 2022

View reviewed changes

sdv/metadata/errors.py Show resolved Hide resolved

fealho commented Aug 2, 2022

View reviewed changes

sdv/metadata/single_table.py Show resolved Hide resolved

fealho commented Aug 2, 2022

View reviewed changes

sdv/metadata/single_table.py Outdated Show resolved Hide resolved

fealho commented Aug 2, 2022

View reviewed changes

tests/unit/metadata/test_single_table.py Outdated Show resolved Hide resolved

fealho force-pushed the issue-879-validate-SingleTableMetadata branch from 14d8108 to 5e4f658 Compare August 2, 2022 17:52

fealho force-pushed the issue-879-validate-SingleTableMetadata branch from c7c61c9 to 06b9dbe Compare August 3, 2022 15:51

fealho commented Aug 3, 2022

View reviewed changes

fealho marked this pull request as ready for review August 3, 2022 17:02

fealho requested a review from a team as a code owner August 3, 2022 17:02

fealho requested review from amontanez24 and pvk-developer and removed request for a team and amontanez24 August 3, 2022 17:02

amontanez24 requested changes Aug 3, 2022

View reviewed changes

amontanez24 approved these changes Aug 3, 2022

View reviewed changes

fealho added 8 commits August 3, 2022 13:29

Add key validation methods (the methods themselves still gotta be rev…

9d7a427

…ised)

Combined errors implementation

b6da514

Finish constraints

d0db252

Add silly constraint solution

3d9a5f7

Add try/catches for validation + add unit and integration tests

4611700

Change MetadataError to InvalidMetadataError

96a1c0e

Simplify tests to one element + fix lint

a54f357

Remove _metadata

9097c5e

fealho added 10 commits August 3, 2022 13:29

Change way _metadata implemented in to_dict

544928f

Add constraint validation + integration test + change _constraints to…

73be37e

… be list of tuple

Fix error message for validate + add error integration test

82bbff0

Fix all constraint related error messages

562ffd3

Add helper _append_error to validate + fix constraint related errors

4cda5b4

Add constraint unit test

4a58faa

Fix lint

c563556

change _metadata[pk] to pk

adfd391

Add tests requested + addresses feedback

df76a4b

Address minor feedback

af34779

fealho force-pushed the issue-879-validate-SingleTableMetadata branch from ee9284f to af34779 Compare August 3, 2022 20:30

pvk-developer approved these changes Aug 3, 2022

View reviewed changes

fealho merged commit aabcaad into V1.0.0.dev Aug 4, 2022

fealho deleted the issue-879-validate-SingleTableMetadata branch August 4, 2022 02:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `validate` to `SingleTableMetadata` #930

Add `validate` to `SingleTableMetadata` #930

fealho commented Jul 29, 2022

fealho Aug 2, 2022 •

edited

Loading

fealho commented Aug 2, 2022

codecov-commenter commented Aug 3, 2022 •

edited

Loading

fealho left a comment

fealho Aug 3, 2022

fealho Aug 3, 2022

fealho Aug 3, 2022

fealho Aug 3, 2022

fealho Aug 3, 2022

fealho Aug 3, 2022

fealho Aug 3, 2022

amontanez24 Aug 3, 2022

amontanez24 left a comment

amontanez24 Aug 3, 2022

fealho commented Aug 3, 2022

amontanez24 commented Aug 3, 2022

amontanez24 left a comment

amontanez24 Aug 3, 2022


		self._metadata['sequence_index'] = column_name
		def _validate_constraint(self, constraint_name, **kwargs):

Add validate to SingleTableMetadata #930

Add validate to SingleTableMetadata #930

Conversation

fealho commented Jul 29, 2022

fealho Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

fealho commented Aug 2, 2022

codecov-commenter commented Aug 3, 2022 • edited Loading

Codecov Report

fealho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

fealho commented Aug 3, 2022

amontanez24 commented Aug 3, 2022

amontanez24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `validate` to `SingleTableMetadata` #930

Add `validate` to `SingleTableMetadata` #930

fealho Aug 2, 2022 •

edited

Loading

codecov-commenter commented Aug 3, 2022 •

edited

Loading