Add ClavaDDPM to SDGym multi table synthesizers

[sarahmish]

original implementation is https://github.com/weipang142857/ClavaDDPM

The important class to review is ClavaDDPM and ClavaDDPMSynthesizer as 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.

[codecov]

Codecov Report

❌ Patch coverage is 81.63558% with 128 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.50%. Comparing base (9b1f0f7) to head (ec384ac).

Files with missing lines	Patch %	Lines
sdgym/synthesizers/clavaddpm.py	81.47%	128 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #630      +/-   ##
==========================================
- Coverage   86.86%   86.50%   -0.37%     
==========================================
  Files          41       42       +1     
  Lines        4409     5104     +695     
==========================================
+ Hits         3830     4415     +585     
- Misses        579      689     +110     

Flag	Coverage Δ
integration	55.83% <79.62%> (+4.14%)	⬆️
unit	76.09% <31.42%> (-7.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pvk-developer]

When fitting I also see the steps (they come from TabDDPM), if verbose is set to False do we need to display them?

[pvk-developer]

During fit there is a point where MLoss and GLoss start to produce nan values, should we stop there for that table?

Step 9000/25000 MLoss: 2.7555 GLoss: 0.9242 Sum: 3.6797
Step 9500/25000 MLoss: nan GLoss: nan Sum: nan
...
Step 24500/25000 MLoss: nan GLoss: nan Sum: nan
Step 25000/25000 MLoss: nan GLoss: nan Sum: nan

[sarahmish]

During fit there is a point where MLoss and GLoss start to produce nan values, should we stop there for that table?

I lean towards keeping it as faithful to the original implementation as possible. Sometimes nan indicates that this model doesn't work for a particular dataset on this synthesizer.

[R-Palazzo]

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:

1. 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 ###################
.
.
.

2. 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]

Do we need to handle the numerical case here?

In l875 and l883 we're skipping the encoder for numerical ids

[sarahmish]

This is additional logic from our side. ClavaDDPM requires id columns to be numeric values, so I added logic to encode non-numerical ids, which is why I added this part during decoding.

If the column is already numeric, the code defaults to expected ClavaDDPM behavior.