search

greenelab / tybalt

Training and evaluating a variational autoencoder for pan-cancer gene expression data
BSD 3-Clause "New" or "Revised" License
162 stars 61 forks source link

Adding Parameter Sweep for Two Hidden Layer Model / Reorganizing nbconverted Scripts #71

Closed gwaybio closed 6 years ago

gwaybio commented 6 years ago

A lot of intermediate files being added in the PR in commit 9997c6d. These files are the result of a parameter sweep of a two hidden layer model (5000 -> 100 -> 100 -> 100 -> 5000). Commit 2b61e96 adds the compiled file with git LFS.

The main file changes include adding options to the parameter sweep script (cd77571) and adding a script that actually trains the models (7abeb7b). This script is very similar to the tybalt notebook.

danich1 commented 6 years ago

I agree with @jaclyn-taroni comments. Just curious as to why a completely new script instead of modifying tybalt_vae.py?

jaclyn-taroni commented 6 years ago

I have the same question as @danich1, that's what I was trying to get at. I thought it might be for notebook reasons?

gwaybio commented 6 years ago

Just curious as to why a completely new script instead of modifying tybalt_vae.py?

I thought it might be for notebook reasons?

Precisely. I think it would be best to keep the nbconverted scripts separate from scripts run consistently. If they are not separate, when a notebook changes, it would get messy quick.

Do you think the repo would be better organized with a scripts/nbcovert/ folder storing these scripts instead?

danich1 commented 6 years ago

I concur on separating out the nbconverted scripts. Better to separate them off to avoid confusion about which is suppose to be a converted script and what is suppose to be a run-consistent script.

gwaybio commented 6 years ago

thanks @danich1 and @jaclyn-taroni - I believe b895ece addresses your concerns. I also believe that some portion of concern is regarding redundancy and I definitely agree.

Portions of this code base are getting entirely too redundant, and this realization prompts refactorization (see #74 for more details).

gwaybio commented 6 years ago

I concur on separating out the nbconverted scripts. Better to separate them off to avoid confusion about which is suppose to be a converted script and what is suppose to be a run-consistent script.

Sounds good. in the next commit, inside this PR (I will also rename the PR). I will reorganize nbconverted scripts.

gwaybio commented 6 years ago

thanks @danich1 and @jaclyn-taroni - ready for review again

jaclyn-taroni commented 6 years ago

@gwaygenomics - Does your output change when setting kernel_initializer='glorot_uniform' in your decoder in b895ece? If so, should probably update those files as well here

gwaybio commented 6 years ago

Does your output change when setting kernel_initializer='glorot_uniform' in your decoder in b895ece?

Good catch. It does not change since glorot_uniform is the default. I add it here for consistency and to remove those concerns.