kundajelab / basepairmodels

MIT License
16 stars 6 forks source link

Option to set number of dilated layers (and sanity check for architecture) #9

Closed mmtrebuchet closed 3 years ago

mmtrebuchet commented 3 years ago

I don't see an option to set the number of dilated convolutions in the train command. It's not a huge deal since I'm defining my own architecture, but it's an easy thing to miss that should be tweakable. On that note, it'd be nice to have the commands do some sanity checking. For example, there's a layer prof_out_crop2match_output that addresses the case where the architecture doesn't reduce down to the desired output width. In my case, if that layer is doing any work, then it means that I mis-defined my network. It'd be nice to have the program emit a warning (if it could silence the tensorflow warnings during startup, then it'd be easier to see. Similarly, when I was making predictions and getting all zeros because I forgot --predict-peaks, it'd be nice to have the program tell me that instead of looking like it's working but then producing the wrong output.

akundaje commented 3 years ago

I dont think specific properties of the architecture e.g. number of dilated layers should be in the train command. The train command should take in a custom architecture defined via whatever architecture configuration file.

-Anshul

On Fri, Feb 5, 2021 at 11:44 AM Charles McAnany notifications@github.com wrote:

I don't see an option to set the number of dilated convolutions in the train command. It's not a huge deal since I'm defining my own architecture, but it's an easy thing to miss that should be tweakable. On that note, it'd be nice to have the commands do some sanity checking. For example, there's a layer prof_out_crop2match_output that addresses the case where the architecture doesn't reduce down to the desired output width. In my case, if that layer is doing any work, then it means that I mis-defined my network. It'd be nice to have the program emit a warning (if it could silence the tensorflow warnings during startup, then it'd be easier to see. Similarly, when I was making predictions and getting all zeros because I forgot --predict-peaks, it'd be nice to have the program tell me that instead of looking like it's working but then producing the wrong output.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kundajelab/basepairmodels/issues/9, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABDWEPEBG5DD7PXIFD5ST3S5RDC7ANCNFSM4XFKO4NQ .

mmtrebuchet commented 3 years ago

I agree with @akundaje on that, though that will require a bit of rejiggering to allow people to easily integrate custom architectures. Maybe have a --architecture-source-file flag that accepts the name of a python file that contains a function genNetwork() taking no arguments (or maybe taking input_seq_len and output_len since those are needed elsewhere) and returning the network to be used. As it stands, I have to symlink model_archs.py out of the basepairmodels code and into my working directory. Not exactly an elegant solution! By requiring the model to be fully specified in the input file, we could remove the architecture-specific flags like --filters from the training command. Maybe we could also have a config file added with --architecture-config-file. The filename would then be passed as a keyword argument to genNetwork(config_file=args.architecture_config_file) for the user's architecture definition code to do with as it pleases.

mmtrebuchet commented 3 years ago

Closing the issue because this is essentially a worse way of solving issue #10 .