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.

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

Known issues regarding implementation. #1

Open
birdx0810 opened this issue Aug 9, 2021 · 3 comments
Open

Known issues regarding implementation. #1

birdx0810 opened this issue Aug 9, 2021 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@birdx0810
Copy link
Owner

birdx0810 commented Aug 9, 2021

The following are some problems with the code I noticed while I was trying to reproduce results of the original paper.

  1. Public dataset and preprocessing methods should be updated for reproducing results in original paper.

  2. The noise sampling method is different from the original code.
    During reproducing the results, using the same noise sampling mechanism fails to fit the original dataset B * torch.uniform((S, Z))

    Z_mb = torch.rand((args.batch_size, args.max_seq_len, args.Z_dim))

Should be changed to something like the code below, and not torch.random((B, S, Z)) to follow a more Wiener Process.

Z_mb  = torch.zeros(B, S, Z)
for idx in batch_size:
    Z_mb[idx] = torch.random(S, Z)
  1. The MSE losses are not respected to sequence length, this would make the model learn padding values when the sequence lengths are not of equal length. This is issue should be highlighted at all calculations of MSE, especially in recovery and supervisor forward pass. This should not be an issue if the public dataset is being used.

    E_loss_T0 = torch.nn.functional.mse_loss(X_tilde, X)

  2. G_loss is wrong in logging, accidental addition of torch.sqrt that is not in original code

    G_loss = np.sqrt(G_loss.item())

  3. Paddings should be added during inference stage.

  4. Original code has a sigmoid activation function. Although Hide-and-Seek competition did not added this if I'm not mistaken, probably heuristics.

    return X_tilde

  5. Arguments if the loss should be instance or stepwise. To be experimented.
    Classification with Discriminator  jsyoon0823/TimeGAN#11 (comment)

@birdx0810
Copy link
Owner Author

These issues have not been resolved as of now. Might take awhile to resolve as I have other things to settle.
Feel free to submit a PR.

@birdx0810 birdx0810 self-assigned this Aug 9, 2021
@birdx0810 birdx0810 added the bug Something isn't working label Aug 9, 2021
@eonu
Copy link

eonu commented Nov 23, 2023

Hi @birdx0810, regarding point no. 5 above, was there a reason for excluding the sigmoid on the recovery network output?

Was it just to allow the network to learn real outputs instead of only (0, 1)? Or some other reason?

@birdx0810
Copy link
Owner Author

birdx0810 commented Mar 1, 2024

Hi @birdx0810, regarding point no. 5 above, was there a reason for excluding the sigmoid on the recovery network output?

Was it just to allow the network to learn real outputs instead of only (0, 1)? Or some other reason?

@eonu Sorry for the late reply, but this code base was derived from the NeurIPS 2020 hide-and-seek privacy challenge hosted by Van Der Schaar Lab, which is the team that proposed the TimeGAN model. I assume that it was just accidentally left out and I only realized it later. It should be added IMO, which is why I listed it in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants