Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add verbose option to TVAE #313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add verbose option to TVAE #313

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ctgan/synthesizers/tvae.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ def __init__(
decompress_dims=(128, 128),
l2scale=1e-5,
batch_size=500,
verbose=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add this parameter to the end? Just in case anybody is passing in all the parameters unnamed, we don't want the wrong values being passed to the wrong parameters

Copy link
Author

@jjmarks jjmarks Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be passed to the end, although this ordering is consistent with CTGAN class,

CTGAN/ctgan/synthesizers/ctgan.py

Line 145 in 2848a42

log_frequency=True, verbose=False, epochs=300, pac=10, cuda=True):

Would it be better to keep the two orderings consistent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I guess it's fine to keep consistent.

epochs=300,
loss_factor=2,
cuda=True
Expand All @@ -122,6 +123,7 @@ def __init__(
self.l2scale = l2scale
self.batch_size = batch_size
self.loss_factor = loss_factor
self.verbose = verbose
self.epochs = epochs

if not cuda or not torch.cuda.is_available():
Expand Down Expand Up @@ -176,6 +178,12 @@ def fit(self, train_data, discrete_columns=()):
optimizerAE.step()
self.decoder.sigma.data.clamp_(0.01, 1.0)

if self.verbose:
print(f'Epoch {i+1}, Loss: {loss.detach().cpu(): .4f},', # noqa: T001
f' Rec loss: {loss_1.detach().cpu(): .4f},',
f' KL loss: {loss_2.detach().cpu(): .4f}',
flush=True)
Comment on lines +182 to +185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind adding unit tests for this case? Just making sure the print out only happens when verbose is true and that the output is as expected

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Are there any active unit tests run for TVAE as a template? I see test_tvae.py but it looks like only placeholder comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don't think there are, although I think a PR with some should be coming in shortly. I would use the tests in this file as an example. I know the tests don't exist for the CTGAN verbose parameter, but we're trying to improve test coverage.

I think you can do something similar to this. Mock print and make sure it gets called with the correct string but only if verbose is True.


@random_state
def sample(self, samples):
"""Sample data similar to the training data.
Expand Down