-
Notifications
You must be signed in to change notification settings - Fork 5
S4 #281
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
base: master
Are you sure you want to change the base?
Conversation
…le_library and use code from s4dd git repo directly
seungchan-an
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S4 integration looks solid, unused models are cleanly commented out, and the loss refactor and NaN early stopping are reasonable. S4 tests are in place and CI is green.
looks good to me.
|
@GuptaVishu2002 I was going to launch a few small runs of RNN vs. Transformer vs. S4 to check that the changes are non-breaking, but I realized I can't actually set the use of Transformer or S4 models from the config.yaml - and src/clm/commands/sample_molecules_RNN.py does not actually import either of these classes. Could you take a look at integrating config.yaml -> Snakemake -> sample_molecules_RNN.py so that the user (here, me) can specify Transformer or S4 in the config and run the whole pipeline with one of these models? We might need to add new parameters to the config, e.g., a "model_type" parameter might be worth considering (since currently the see also issue #283 |
|
Update on this: tested the S4 implementation with the NPS training set. The model is not outperforming the LSTM by any means but seems to be doing reasonably well, ruling out any major issues in the implementation. The Transformer on the other hand is failing immediately at the |
…r does not maintain recurrent state. Also add torch.cuda.empty_cache() and torch.no_grad() for sampling to GPU memory management
Fresh start trying to merge #275