[DEVELOP] Adding TAD class #34
Conversation
|
For all of the reasons that code review is important including the opportunity to catch certain bugs, identify new solutions, and making sure code is reusable across the lab (and hopefully also the field), it seems worth going through review. This isn't an inordinately large amount of code. I bet that if you asked @dongbohu he would be happy to take a look. I'm fine with the develop branch of this repo. You might also be able to get help from @dongbohu to make this repo into a python package. I'm not sure if he has the capacity but it seems like it'd be worth asking. |
| import yaml | ||
|
|
||
|
|
||
| def config_yaml(config_file): |
There was a problem hiding this comment.
So I suppose this module will be used in the future by users to read in a yaml config file?
| # Load configuration | ||
| with open(config_file, "r") as stream: | ||
| config = yaml.load(stream) | ||
|
|
There was a problem hiding this comment.
Do you have a template yaml file? I guess some error handling logic will be needed in the future to validate the input yaml file.
There was a problem hiding this comment.
This isn't currently used, but will be in the future. I will add error handling in a future PR (see #35)
tad_pathways/config.py
Outdated
| @@ -0,0 +1,31 @@ | |||
| """ | |||
| Gregory Way 2018 | |||
tad_pathways/genes/genelist.py
Outdated
| @@ -0,0 +1,167 @@ | |||
| """ | |||
| Gregory Way 2018 | |||
tad_pathways/genes/genelist.py
Outdated
| """ | ||
| Gregory Way 2018 | ||
| TAD Pathways | ||
| scripts/genes/genelist.py |
There was a problem hiding this comment.
This path doesn't match the actual location.
tad_pathways/tad.py
Outdated
|
|
||
| def build_custom_tad_genelist(self): | ||
| command_list = [ | ||
| "python", |
There was a problem hiding this comment.
Is scripts/build_custom_tad_genelist.py compatible for Python 2 or 3 (or both)? By default, python command calls python 2 on many (if not most) operating systems.
There was a problem hiding this comment.
python 3 - should I change the call to "python3"?
There was a problem hiding this comment.
If this package is for Python 3 only, then it is a good idea to change it to python3,
tad_pathways/tad.py
Outdated
| output_pval_file = os.path.join( | ||
| self.base_dir, "{}_pvals.tsv".format(self.snp_list_name) | ||
| ) | ||
| if os.path.exists(output_pval_file): |
There was a problem hiding this comment.
If you want, lines 136-139 can be simplified as:
return os.path.exists(output_pval_file)
tad_pathways/tad.py
Outdated
| def get_evidence(self): | ||
| # The first command builds the evidence | ||
| command_list = [ | ||
| "python", |
There was a problem hiding this comment.
Same question on Python 2/3 compatibility.
tad_pathways/tad.py
Outdated
|
|
||
| # The second command summarizes this evidence | ||
| command_list = [ | ||
| "python", |
There was a problem hiding this comment.
Same question on python 2/3 compatibility.
tad_pathways/tad.py
Outdated
| webgestalt_exists = self.check_webgestalt() | ||
| if webgestalt_exists: | ||
| self.get_evidence() | ||
| return 1 |
There was a problem hiding this comment.
Just curious, the return value of this method will be used later?
There was a problem hiding this comment.
Hmm, doesn't look like I use this.. Not sure why its here. Maybe some functionality that I decided not to use. I will remove for now
|
Proposed next steps to complete the enrichment analysis:
We are working on this-- any thoughts before we proceed are welcome. |
update documentation, update all_pathways argument, streamline return statements
update documentation, remove remove_hla_tad argument from compile()
|
@dongbohu - ready again for re-review! |
Adding method to facilitate a TAD_Pathways analysis. The method calls all the scripts in order of the tad_pathways pipeline given a file storing SNP info.
This is in preparation of Diana's Longitudinal BMD paper. Not really sure of where this should all live...
The TAD_Pathways portion of the longitudinal BMD analysis consists of three steps:
At this point, nearly all of the functionality lives on the
developbranch in this repo. All of it has been reviewed already (except this PR) (see #21, #22, #25, #26, #30, #31, #33). My original goal was to make this repo a python package so that Diana's analysis could live in an independent repo - but this goal is likely to require several more hours of work.An alternative is to add the longitudinal BMD analysis here in a separate branch (can branch off of
develop) and generate a release on this specific branch. I think this will take the least amount of time.There is also the element of code review. I certainly don't want to take up any time of anyone in the lab...
Thoughts @cgreene ?