-
Notifications
You must be signed in to change notification settings - Fork 320
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.
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
Add SingleTableMetadata visualization #1535
Add SingleTableMetadata visualization #1535
Conversation
Codecov ReportPatch coverage:
❗ 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 #1535 +/- ##
==========================================
+ Coverage 96.36% 96.39% +0.02%
==========================================
Files 49 49
Lines 3937 3965 +28
==========================================
+ Hits 3794 3822 +28
Misses 143 143
☔ View full report in Codecov by Sentry. |
sdv/metadata/single_table.py
Outdated
@@ -477,6 +479,60 @@ def validate(self): | |||
+ '\n'.join([str(e) for e in errors]) | |||
) | |||
|
|||
def visualize(self, show_table_details='full', output_filepath=None): | |||
"""Create a visualization of the multi-table dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single-table
sdv/metadata/single_table.py
Outdated
|
||
Args: | ||
show_table_details (str): | ||
If 'full', show the column names, primary, alternate and sequence keys are all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove show
after If 'full'
. Also, review the docstrings, they mention table_names when I don't think we have them for single_table.
Also, shouldn't None be an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@npatki None is an option for the multi-table visualization but in that case all it shows is the table name. That doesn't really make sense in single-table so should it be an option or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not add it because it didn't make sense (empty box ?) ... In multi table I see a point of None
since you can see the diagram.
sdv/metadata/single_table.py
Outdated
|
||
Args: | ||
show_table_details (str): | ||
If 'full', show the column names, primary, alternate and sequence keys are all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@npatki None is an option for the multi-table visualization but in that case all it shows is the table name. That doesn't really make sense in single-table so should it be an option or not?
b9e1438
to
1e8fd04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Resolves #1517