Config files handling #22
Conversation
There was a problem hiding this comment.
One design question is whether we want to modify SINGE_Example.m to only support parameters in config files or have one example wrapper showing SINGE with parameters in the config files (SINGE_cmd.m) and another showing parameters in the MATLAB file (the previous style of SINGE_Example.m). Should we have have both? In both cases, they would only be wrappers for SINGE.m.
What changed in tf.mat? Is this what is causing the Travis CI build to fail with the error:
Error using InputOutputModel/subsref (line 43)
Subscript no. 1 is out of range.
Error in adjmatrix2edgelist (line 16)
Error in SCINGE (line 33)
Error in SCINGE_Example (line 36)
I'd prefer to revert the tf.mat changes and save them for a different pull request so we can confirm the new way of running SINGE still produces the exact same output.
SINGE_Example.m
Outdated
| @@ -0,0 +1,47 @@ | |||
| %% Simple example that runs SCINGE for two replicates of two hyperparameter settings | |||
There was a problem hiding this comment.
How much of this file changed? If you delete the old SCINGE_Example.m in the same commit where you add this file, git may recognize that you renamed the file. That would make comparing the new and old version easier.
Please convert the name here as well
| end | ||
|
|
||
| %% Import list of parameter combinations | ||
| fid = fopen('SINGE_params.cfg'); |
There was a problem hiding this comment.
We should support specifying the config file names as a command line argument. This could be the default name or we could require both types of config files to be provided before running. That way we can compile this file one and let users run on different datasets without needing to recompile.
| %% Import list of parameter combinations | ||
| fid = fopen('SINGE_params.cfg'); | ||
| temp = fgetl(fid); | ||
| while any(temp~=-1)||isempty(temp) |
There was a problem hiding this comment.
How much error handling do we need here? I believe that parseParams.m will provide default values for most of the parameters. Are the data files the only required configuration file values?
| %% Specify Path to Input data and path to Output folder, gene_list and number of subsampled replicates | ||
| fid = fopen('SINGE_IO.cfg'); | ||
| temp = fgetl(fid); | ||
| while any(temp~=-1)||isempty(temp) |
There was a problem hiding this comment.
Same question as above regarding error handling and required config file values.
| @@ -0,0 +1,4 @@ | |||
| Data data1/X_SCODE_data.mat | |||
| outdir Output | |||
| num_replicates 2 | |||
There was a problem hiding this comment.
Should we call this file SINGE_config.cfg, SINGE_options.cfg, or SINGE_settings.cfg? Most of it is I/O related, except the number of replicates.
Before merging, we'll need to update the readme to describe this file and how to run SINGE with it. Is this space separated or whitespace-separated? Tab-separated may be safest so we can support Windows file paths.
| @@ -0,0 +1,19 @@ | |||
| 1 ID 541 | |||
There was a problem hiding this comment.
Should we make this file tab-separated as well? I see multiple spaces instead of tabs.
We will also document this file in the readme.
code/SINGE.m
Outdated
| @@ -0,0 +1,42 @@ | |||
| function [ranked_edges, gene_influence] = SCINGE(gene_list,Data,outdir,num_replicates,param_list) | |||
| % [ranked_edges, gene_influence] = SCINGE(gene_list,Data,outdir,num_replicates,param_list) | |||
| % Standalone SCINGE implementation. | |||
code/SINGE.m
Outdated
| @@ -0,0 +1,42 @@ | |||
| function [ranked_edges, gene_influence] = SCINGE(gene_list,Data,outdir,num_replicates,param_list) | |||
There was a problem hiding this comment.
Let's remove the old SCINGE.m if it is no longer needed. How much of this file changed? Just the name?
|
Can we close this now that it is replaced by #37? |
|
Closed since being replaced by #37. |
Set of changes to incorporate a config file for setting parameters and identifying input files and output directories, etc. Also changed the name of SCINGE_Example.m and SCINGE.m to SINGE equivalents.
Closes #17