Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
==========================================
- Coverage 86.86% 86.43% -0.44%
==========================================
Files 41 42 +1
Lines 4409 5107 +698
==========================================
+ Hits 3830 4414 +584
- Misses 579 693 +114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pvk-developer
left a comment
There was a problem hiding this comment.
When fitting I also see the steps (they come from TabDDPM), if verbose is set to False do we need to display them?
|
During fit there is a point where |
I lean towards keeping it as faithful to the original implementation as possible. Sometimes |
There was a problem hiding this comment.
This is looking good!
I’m still reviewing the logic and checking that it aligns with their implementation. In the meantime, I’m thinking about two things:
- For the structure, we could either:
- Define a clavaddpm folder where we keep files such as pipeline_modules.py and preprocess_utils.py, along with our SDGym logic.
- Keep everything in one file, but be more explicit about what comes from their implementation versus what we added. It could look something like:
######################### SDGym extra logic #########################
def get_group_data_dict(np_data, group_id_attrs=[0]):
"""Grouping dictionary from pipeline_utils.py."""
group_data_dict = {}
data_len = len(np_data)
for i in range(data_len):
row_id = tuple(np_data[i, group_id_attrs])
if row_id not in group_data_dict:
group_data_dict[row_id] = []
group_data_dict[row_id].append(np_data[i])
return group_data_dict
######################## From preprocess_utils.py ###################
def get_domain(df, id_cols, discrete_cols):
"""Build the ``{col: {'size', 'type'}}`` domain of a table from preprocess_utils.py."""
domain = {}
for col in df.columns:
if col in discrete_cols:
domain[col] = {'size': len(df[col].unique()), 'type': 'discrete'}
elif col not in id_cols:
domain[col] = {'size': len(df[col].unique()), 'type': 'continuous'}
return domain
.
.
.
######################## From pipeline_modules.py ###################
.
.
.
- Would it be possible to have an integration test that runs the SDGym ClavaDDPM implementation against their original implementation and checks that the results are the same or similar? It would be nice to have, although I’m not sure how feasible it is. It might require adding their repo as a test dependency.
R-Palazzo
left a comment
There was a problem hiding this comment.
LGTM!
Looking at the GCP results, there are various errors for the private datasets (rel-bench datasets, instacart_marketbasket_ml, etc.).
To know whether the errors are coming from their code or from the extra logic we added, we would have to look into the log files, right? (Not the most convenient here)
original implementation is https://github.com/weipang142857/ClavaDDPM
The important class to review is
ClavaDDPMandClavaDDPMSynthesizeras these are the wrappers I wrote. I've written other code pieces, if it's taken from the source code directly, I mention "from" in the docstrings.