-
Notifications
You must be signed in to change notification settings - Fork 35
Enable caching of compiled guvectorize functions #364
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor query
@@ -121,9 +121,7 @@ def diversity( | |||
|
|||
# c = cohorts, k = alleles | |||
@guvectorize( # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any good reason not to have cache=True
for our use-case? Perhaps it's worth us having our own local guvectorize
decorator that does this by default?
This is the sort of thing that's hard to test for and easy to forget, so maybe it's worth a little indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? It's not hard to add and now we have the convention started it is likely to be propagated in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #371
That will be a good addition, I think. |
This is a really nice improvement, by the way. 👍 |
Codecov Report
@@ Coverage Diff @@
## master #364 +/- ##
==========================================
+ Coverage 95.39% 95.41% +0.01%
==========================================
Files 31 31
Lines 2108 2116 +8
==========================================
+ Hits 2011 2019 +8
Misses 97 97
Continue to review full report at Codecov.
|
Fixes #363
BTW @aktech I noticed that the
guvectorize
metric functions don't havenopython=True
- do you think we should add it? (I could do it in this PR.)