-
Notifications
You must be signed in to change notification settings - Fork 541
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
[0.2.0] Metadata Implementation #81
Conversation
for more information, see https://pre-commit.ci
Currently, metadata is in the design stage. I will probably complete the specific design tomorrow, I plan to initiate a discussion when completing the design. |
Apply Sweep Rules to your PR?
|
We can ignore the multi-table scenario for now and get the single table fields done. The table-to-table relationship (A.a vs. B.a) can be considered separately from the field (a,b,c,d) case within the table. I'd like to know what is necessarily contained in a single table Metadata, such as the currently implemented discrete columns: https://github.com/hitsz-ids/synthetic-data-generator/blob/main/sdgx/data_models/inspectors/discrete.py |
Yes, we will first update the metadata of a single table. Currently, the metadata of a single table has the following new information (please help me check if there are any gaps):
|
I think this is very necessary. We may start from support 5 basic types:
|
However, the multi-table scenario is also the focus of our work. We have faced the need for multi tables in some actual work, and existing solutions cannot solve their needs. After finishing single table metadata, I will implement metadata for multi tables based on |
Metadata will develop based on 0.1.0.
for more information, see https://pre-commit.ci
Seems no need to intro a submodule as
|
It’s okay, there aren’t many new modules in metadata. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
==========================================
- Coverage 76.29% 75.87% -0.42%
==========================================
Files 52 58 +6
Lines 2054 2230 +176
==========================================
+ Hits 1567 1692 +125
- Misses 487 538 +51 ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Use a single list for single or composite primary key. Co-authored-by: Zhongsheng Ji <9573586@qq.com>
relationship_cnt = len(self.relationships) | ||
metadata_cnt = len(self.metadata_dict.keys()) | ||
if metadata_cnt != relationship_cnt + 1: | ||
raise ValueError("Number of tables should corresponds to relationships.") |
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.
Can we add a new exception in sdgx/exceptions.py
? This will help us differentiate between what are expected exceptions.
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.
This is something I didn't consider, I'll add new exceptions MultiTableCombineError
and MetadataError
in sdgx.exceptions
.
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.
For better unit testing and bug fixing, I'd suggest synchronizing the main branch, which also includes Exceptions
with error codes and exit codes
I think the current design of relationship is sufficient to describe existing multi-table relationships. In synthetic data, the multi-table relationships we currently implement are limited to “one-to-one” or “one-to-many”, that is, a parent table is associated with different child tables through foreign keys. In the current design, relationship is like a line in this correspondence, and each relationship only describes the relationship between two tables. We use multi-table combiner to combine table's meatadata and relationships. |
Sounds fine, looking forward to more commits! |
Key are described using list, which is compatible with single or composite foreign key. Co-authored-by: Zhongsheng Ji <9573586@qq.com>
for more information, see https://pre-commit.ci
Unit testing also needs to be implemented.
for more information, see https://pre-commit.ci
sdgx/data_models/metadata.py
Outdated
# check missing columns | ||
for each_column in self.column_list: | ||
if each_column not in all_dtype_columns: | ||
raise MetadaError(f"Undefined data type for column {each_column}.") | ||
|
||
# check unfamiliar columns in dtypes | ||
for each_dtype_column in all_dtype_columns: | ||
if each_dtype_column not in self.column_list: | ||
raise MetadaError(f"Found undefined column: {each_dtype_column}.") | ||
|
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.
Maybe we can use set(self.column_list) - set(all_dtype_columns)
here, which we can indicate all invalid keys once.
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.
And maybe we can rename MetadaError
to MetadataInvalidError
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.
Maybe we can use
set(self.column_list) - set(all_dtype_columns)
here, which we can indicate all invalid keys once.
This is a more concise way of writing.
The current implementation is because the error information raised in these two cases is different, which may make it easier for users to locate the two error types.
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.
The current implementation is because the error information raised in these two cases is different, which may make it easier for users to locate the two error types.
There is no conflict, just use two if
-expressions instead of two for-if
s.
Co-authored-by: Zhongsheng Ji <9573586@qq.com>
Unit testing also needs to be implemented.
for more information, see https://pre-commit.ci
still some cases not completed yet.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Could you please add some tests on I think this PR can be merged at the moment, and one or more PRs may be needed next to intro |
No problem, I will merge after completing the tests of |
Description
The metadata component for single table and multi table.
Motivation and Context
This metadata is used to describe:
Also, metadata will also provide methods to:
How has this been tested?
Types of changes
Checklist: