Refactor tfa #17
Refactor tfa #17
Conversation
Current coverage is 98.09% (diff: 99.13%)@@ master #17 diff @@
==========================================
Files 14 16 +2
Lines 880 996 +116
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 862 977 +115
- Misses 18 19 +1
Partials 0 0
|
inferelator_ng/tfa.py
Outdated
| self.exp_mat = exp_mat_halftau | ||
|
|
||
| def tfa(self, noself = True, dup_self = True): | ||
| tfwt = self.prior.abs().sum(axis = 0) > 0 |
There was a problem hiding this comment.
Please use more descriptive variable names or add a comment explaining what the variable represents. Of course you can make exceptions for conventional names like x, y, dt, etcetera.
There was a problem hiding this comment.
I specified the comments. Please let me know if it's better. Thanks!
There was a problem hiding this comment.
It's not immediately clear what tfwt represents, especially if your are not fluent in numpy conventions.
I'd like better names or comments explaining the computations and intension, please.
|
Please also provide test cases for these contributions. Thanks! |
|
Hey, I've just taken a brief look at this. I agree that we will need tests before this can be merged in. Ideally, the tests could test all combinations of noself and dup_self. Question: Does the tfa.R file need to be committed? I don't think there's a right answer to this question. But it does seem redundant to keep the tfa.R code in this repo when it's not being called by the python implementation of tfa.py and when it is already in version control thanks to Aaron's most recent pull request #16 |
inferelator_ng/tfa.py
Outdated
| # find non-duplicated TFs that are also present in target gene list | ||
| ndTFs = list(set(self.prior.columns.values.tolist()).difference(dTFs)) | ||
|
|
||
| if noself: |
There was a problem hiding this comment.
Hi Ren!
Some minor style nits: variable names should avoid capital letters, e.g. dTFs, and use underscores where appropriate (noself -> no_self).
Echoing @AaronWatters above, one convention that Hammer Lab uses is to avoid abbreviations wherever possible. While this makes the coding a little more verbose, the result is far more readable. E.g. dTFs -> duplicate_tfs, ndTFs -> non_duplicated_tfs, no_self -> no_self_interaction.
…sues with the tfa.py script
…ded final combination of dup_self and no_self
wrote scaffolding for 3 quick tests and fixed obvious nomenclature is…
inferelator_ng/tests/test_tfa.py
Outdated
| priors.columns = ['t1', 't2', 't3', 't4'] | ||
| priors.index = ['g1', 't2', 'g3', 'g4', 'g5'] | ||
| self.tfa_python = tfa.TFA(priors, exp, exp) | ||
|
|
There was a problem hiding this comment.
Added column and row names for exp and prior matrices as they are needed to identify self interactions.
Prior matrix now contain duplicated TFs, zero TFs and self interaction.
…oving the no_self flag until further notice (this dramatically simplifies things)
* 'master' of github.com:ryi06/inferelator_ng: test setup_max
…ents and fewer lines
Ndv tfa nonzero tfs
remove num2words
Refactor tfa
Refactor tfa
5 participants
Results from tfa.py was compared with results from tfa.R manually...
I will work on the unit test next