-
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
Add base model for multi-table statistic model, change single-table base class location #102
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ds/synthetic-data-generator into refactoring-base-model-partitial
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 #102 +/- ##
==========================================
+ Coverage 78.68% 78.99% +0.30%
==========================================
Files 61 62 +1
Lines 2599 2680 +81
==========================================
+ Hits 2045 2117 +72
- Misses 554 563 +9 ☔ 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
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Nice PR, and I have a couple questions:
|
Regarding this question, it mainly comes from my lack of understanding of
this is indeed inconsistent with the purpose of a statistical model. |
Offer fuel on snowy weather! Multi-table model is what we want! |
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 validation in metadata is ok!
There will be some minor modifications before merge. |
- remove `DataAccessType` - remove `pydantic.BaseModel` - set two mutually exclusive parameters `use_raw_data` and `use_dataloader` in `sdgx.models.base.SynthesizerModel` - some other necessary modifications
for more information, see https://pre-commit.ci
I removed the I removed the In existing architecture, I think it's OK to simply set In the future, we can add support for these two data access methods . (I leave this to future, not in this PR). Currently, this modification does not affect the use of existing models. |
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 prefer to merge it now and make further improvements in the implementation of multi-table simulations.
Description
Motivation and Context
The main purpose of this change is to adapt to multi-table synthetic data scenarios
How has this been tested?
A test case has been provided for the method of base class.
Types of changes
Checklist: