Skip to content
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

PEP8, refactor IEDB #36

Merged
merged 7 commits into from
Mar 18, 2015
Merged

PEP8, refactor IEDB #36

merged 7 commits into from
Mar 18, 2015

Conversation

iskandr
Copy link
Contributor

@iskandr iskandr commented Mar 18, 2015

  • Using single instance of datacache.Cache instead of sprinkling hard-coded subdir arguments around multiple download points
  • Split iedb.py into a sub-package called iedb with modules mhc, tcell, alleles.
  • Added a memoization to major functions in iedb
  • Lots of misc. PEP8 fixes
  • Got rid of tcga peptides functions, which used hackish MAF amino acid change parsing (can use Varcode in the future)

Review on Reviewable

@arahuja
Copy link
Contributor

arahuja commented Mar 18, 2015

Comments from the review on Reviewable.io


pepdata/common.py, line 38 [r1] (raw file):
Does it matter here if one of the two is hashable?


pepdata/hiv_frahm.py, line 31 [r1] (raw file):
Does read_csv work here or should it be read_html?


pepdata/iedb/alleles.py, line 86 [r1] (raw file):
Is this just debugging?


pepdata/iedb/common.py, line 35 [r1] (raw file):
Is there a reason to do the aggregation individually and building a new dataframe instead of df.agg({..})?


pepdata/iedb/mhc.py, line 123 [r1] (raw file):
Can this be configurable?


pepdata/iedb/mhc.py, line 232 [r1] (raw file):
quotes?


pepdata/iedb/mhc.py, line 235 [r1] (raw file):
quotes?


pepdata/iedb/tcell.py, line 151 [r1] (raw file):
Can this just be a map on the series?
mhc.map(lambda allele: 1 if ...)

maybe no reason to change it.


@iskandr
Copy link
Contributor Author

iskandr commented Mar 18, 2015

Comments from the review on Reviewable.io


pepdata/common.py, line 38 [r1] (raw file):
I was thinking of trying to handle that, do you think it's worth it?


pepdata/hiv_frahm.py, line 31 [r1] (raw file):
This is an oddity of how I wrote datacache, it'll pull a table out of an HTML file by default and give you a CSV. I changed the filename to be "frahm.csv" and added an issue to datacache to not perform all that magic under the hood: openvax/datacache#24


pepdata/iedb/alleles.py, line 86 [r1] (raw file):
Yep, getting rid of it


pepdata/iedb/common.py, line 35 [r1] (raw file):
I guess the alternative is something like:

agg = groups.agg({'value' : np.mean, 'count' : lambda x: len(x), 'pos' : np.sum})
agg['neg'] = agg['count'] - agg['pos']
if min_count:
    mask = agg['counts'] >= min_count
    agg = agg[mask]
return agg

Would that work and do you think it's clearer?


pepdata/iedb/mhc.py, line 123 [r1] (raw file):
Added a only_standard_amino_acids option which defaults to True


pepdata/iedb/tcell.py, line 151 [r1] (raw file):
That logic is a bit too large to fit into a lambda, so I'd have to add an inline function, which I'm not sure makes the code any clearer.


@arahuja
Copy link
Contributor

arahuja commented Mar 18, 2015

Comments from the review on Reviewable.io


pepdata/iedb/common.py, line 35 [r1] (raw file):
Actually maybe I misunderstood what was going on here - was this getting averages and counts for many columns? If so agg probably won't be clearer

it'd be .agg({'col': [np.mean, np.sum'], 'col2': ..

I guess its not clear what is in peptides so maybe a comment?


@iskandr
Copy link
Contributor Author

iskandr commented Mar 18, 2015

Comments from the review on Reviewable.io


pepdata/iedb/common.py, line 35 [r1] (raw file):
Added a docstring to hopefully clear up what this function actually does.


iskandr added a commit that referenced this pull request Mar 18, 2015
@iskandr iskandr merged commit 17df772 into master Mar 18, 2015
@iskandr iskandr deleted the refactor_iedb branch March 18, 2015 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants