Add verbose option to TVAE #313
Conversation
|
Hi @jjmarks! Thanks for your interest in contributing to the SDV software. In order to review and approve your code changes, we require that you read and sign our new Contributor License Agreement (CLA). To request a CLA, please fill out the required information in this form: https://bit.ly/sdv-cla-form Once we receive your submission, we'll get back to you with more details. Thanks, and feel free to respond here if you have any questions. |
|
Hi @jjmarks, this is just a confirmation that we've received your signed CLA. A member of the SDV team will now be able to review/merge your PR. Thanks! |
| @@ -110,6 +110,7 @@ def __init__( | |||
| decompress_dims=(128, 128), | |||
| l2scale=1e-5, | |||
| batch_size=500, | |||
| verbose=False, | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Can be passed to the end, although this ordering is consistent with CTGAN class,
CTGAN/ctgan/synthesizers/ctgan.py
Line 145 in 2848a42
Would it be better to keep the two orderings consistent?
There was a problem hiding this comment.
Hmm I guess it's fine to keep consistent.
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Resolves #307.
Resolves #269.
Print reconstruction loss and KL divergence loss terms. Does not store values or use progress bar.