Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add validate_data for multi-table #1541

Merged
merged 9 commits into from
Aug 21, 2023
Merged

Add validate_data for multi-table #1541

merged 9 commits into from
Aug 21, 2023
+286 −152

Conversation

fealho
Copy link
Member

@fealho fealho commented Aug 15, 2023

Resolve #1518 for multi table.

fealho
fealho commented Aug 15, 2023
View reviewed changes
sdv/metadata/multi_table.py Outdated
return [error_msg] if error_msg else []

def validate_with_data(self, data):
"""Validate the data matches the metadata."""
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: improve docstring

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2023 •
edited
Loading

Codecov Report

Patch coverage: 98.48% and project coverage change: -0.38% ⚠️

Comparison is base (cc6fa97) 96.44% compared to head (4879631) 96.07%.
Report is 2 commits behind head on master.

❗ Current head 4879631 differs from pull request most recent head 10eba82. Consider uploading reports for the commit 10eba82 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1541      +/-   ##
==========================================
- Coverage   96.44%   96.07%   -0.38%     
==========================================
  Files          48       48              
  Lines        4025     4052      +27     
==========================================
+ Hits         3882     3893      +11     
- Misses        143      159      +16
Files Changed Coverage Δ
sdv/multi_table/base.py 91.00% <93.33%> (-7.58%) ⬇️
sdv/metadata/multi_table.py 99.50% <100.00%> (+0.06%) ⬆️
sdv/multi_table/hma.py 80.12% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fealho fealho marked this pull request as ready for review August 15, 2023 05:40
@fealho fealho requested a review from a team as a code owner August 15, 2023 05:40
@fealho fealho requested review from frances-h, amontanez24 and a team and removed request for a team August 15, 2023 05:40
@fealho fealho force-pushed the issue-518-validate-with-data-multitable branch from 15375c2 to fad0ee1 Compare August 16, 2023 03:35
amontanez24
amontanez24 reviewed Aug 16, 2023
View reviewed changes
sdv/metadata/multi_table.py Outdated
Comment on lines 548 to 549
if table_synthesizers:
table_synthesizers[table_name].validate(table_data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this out of the metadata. It's a strange cyclic relationship. I think we can just do the case in the else and then do the case in the if only in the MultiTableSynthesizer. That would also make it clear why MultiTableMetadata.validate_data isn't being called in the synthesizer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue of doing that is the code gets duplicated, which is why I took this approach. It is confusing though, so I guess I'll just leave the duplicated code there.

@fealho fealho force-pushed the issue-518-validate-with-data-multitable branch from fad0ee1 to c9b4fc5 Compare August 17, 2023 20:36
Base automatically changed from issue-1518-validate-with-data-method to master August 18, 2023 01:58
@fealho fealho force-pushed the issue-518-validate-with-data-multitable branch from c9b4fc5 to 0df9f1b Compare August 18, 2023 02:22
@fealho fealho changed the title Add validate_with_data for multi-table Add validate_data for multi-table Aug 18, 2023
amontanez24
amontanez24 reviewed Aug 18, 2023
View reviewed changes
sdv/multi_table/base.py Outdated Show resolved Hide resolved
sdv/multi_table/base.py Outdated Show resolved Hide resolved
amontanez24
amontanez24 approved these changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good!

frances-h
frances-h approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@frances-h frances-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@fealho fealho merged commit be2f71e into master Aug 21, 2023
37 checks passed
@fealho fealho deleted the issue-518-validate-with-data-multitable branch August 21, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amontanez24 amontanez24 amontanez24 approved these changes

@frances-h frances-h frances-h approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add validate_data method to Metadata
4 participants
@fealho @codecov-commenter @amontanez24 @frances-h