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

Getting rid of memoization on many properties and methods #151

Merged
merged 2 commits into from
Jun 7, 2016

Conversation

iskandr
Copy link
Contributor

@iskandr iskandr commented Jun 7, 2016

Partially addresses: #150

We're still leaking an unbounded amount of memory to memoization but now it's at least being centralized through a few "important" methods such as those which get Gene, Transcript, and Exon objects by ID or the database query helpers.


This change is Reviewable

@@ -55,7 +55,7 @@ def __init__(self, gtf_path, cache_directory_path=None):

def __eq__(self, other):
return (
isinstance(other, GTF) and
(other.__class__ == GTF) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not important, but just noting I think this change means you can't subclass GTF and have equality work. Not sure if that's intentional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to imagine when we'd want a sub-class of GTF to equal to a GTF instance, seemed safer to restrict equality to exactly the same class. .

@timodonnell
Copy link
Contributor

LGTM

@iskandr iskandr merged commit 03563c0 into master Jun 7, 2016
@iskandr iskandr deleted the less-memoization branch June 7, 2016 21:03
@coveralls
Copy link

coveralls commented Jun 7, 2016

Coverage Status

Coverage decreased (-0.3%) to 80.026% when pulling a236762 on less-memoization into 1df3c22 on master.

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.

None yet

4 participants