Update vaegan.py

[Saran-nns]

Test run of the model is available at Notebook.

Todo: Update vaegan trainer to current traja trainer

[codecov-io]

Codecov Report

Merging #48 (0e4aa9c) into master (e3dcf25) will decrease coverage by 6.82%.
The diff coverage is 12.08%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   72.40%   65.57%   -6.83%     
==========================================
  Files          25       25              
  Lines        2551     2870     +319     
==========================================
+ Hits         1847     1882      +35     
- Misses        704      988     +284     

Impacted Files	Coverage Δ
traja/models/generative_models/vaegan.py	12.04% <12.08%> (-26.42%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3dcf25...0e4aa9c. Read the comment docs.

[JustinShenk]

LGTM

[JustinShenk]

@WolfByttner does this need revisions or is ready to merge?

[WolfByttner]

This seems quite old. The models have been split up over several files, since they are used in both the AE and VAE models.

[WolfByttner]

@Saran-nns which style settings are you using? You changed all single quotes to double quotes. You also added implicit decimals.

We need to define a style for this repo and set everyone up with it.

[Saran-nns]

I am using "black". Yes, it is an older version and is not compatible with the recent trainer as well due to the complexity compared to AE and VAEs. It is better to keep it as an independent module with its own trainer.

[JustinShenk]

Formatting with black is enforced with .pre-commit-config.yaml, or at least it should be.

[Saran-nns]

@WolfByttner Single to double quotes look reasonable since the dict keys have to be in double-quotes, but I never know formatter even cares about decimal point 0. --> 0.0

[JustinShenk]

@WolfByttner do you have any updated instructions for this or can Saran focus on fixing the merge conflict?

[WolfByttner]

@JustinShenk, @Saran-nns not for VAEGAN itself. That model is still independent of everything else.

If there are modifications to trainer, we should probably coordinate them. @Saran-nns, feel free to come up with a suggestion.

[Saran-nns]

@JustinShenk @WolfByttner Yes, vaegan requires intense patching over trainer. Current trainer seems to be good generalized module. To develop and maintain models like vaegan, we could have another sub directory "advanced_models" and drop such models (like Inverse RL) there with their own custom trainers. Somethnig as traja.generative_models.vaegan.trainer(*args,**kwargs) while __init__ at directory generative_models import modules from subdirectory advanced_models
i will make the changes and update this pr

[JustinShenk]

Do you have an estimate for when this will be updated @Saran-nns?

[Saran-nns]

This might take a month to update and test the vaegan network under the recent data loader.

[JustinShenk]

Okay, thanks for the update.

…

On Tue, May 4, 2021 at 4:09 PM Saranraj Nambusubramaniyan < ***@***.***> wrote: This might take a month to update and test the vaegan network under the recent data loader. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACOLMZH2RCH7MXP77OW4SFDTMBIABANCNFSM4W2ZFQBA> .

[JustinShenk]

@WolfByttner any feedback on the review?

[Saran-nns]

@JustinShenk This is not feature complete and need several compatibility checks. Self reviewed ;)