search

birdx0810 / timegan-pytorch

This repository is a non-official implementation of TimeGAN (Yoon et al., NIPS2019) using PyTorch.
MIT License
80 stars 35 forks source link

Known issues regarding implementation. #1

Open birdx0810 opened 3 years ago

birdx0810 commented 3 years ago

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)) https://github.com/birdx0810/timegan-pytorch/blob/7d0445540aed9f9e718c122d537535a172dd2f4f/models/utils.py#L114

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. https://github.com/birdx0810/timegan-pytorch/blob/7d0445540aed9f9e718c122d537535a172dd2f4f/models/timegan.py#L414

  2. G_loss is wrong in logging, accidental addition of torch.sqrt that is not in original code https://github.com/birdx0810/timegan-pytorch/blob/7d0445540aed9f9e718c122d537535a172dd2f4f/models/utils.py#L120

  3. Paddings should be added during inference stage. https://github.com/birdx0810/timegan-pytorch/blob/7d0445540aed9f9e718c122d537535a172dd2f4f/models/utils.py#L271

  4. Original code has a sigmoid activation function. Although Hide-and-Seek competition did not added this if I'm not mistaken, probably heuristics. https://github.com/birdx0810/timegan-pytorch/blob/7d0445540aed9f9e718c122d537535a172dd2f4f/models/timegan.py#L150

  5. Arguments if the loss should be instance or stepwise. To be experimented. https://github.com/jsyoon0823/TimeGAN/issues/11#issuecomment-713323225

birdx0810 commented 3 years ago

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.

eonu commented 12 months ago

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 commented 8 months ago

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.