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

Faster CLR Calculation #5608

Merged
merged 4 commits into from
Dec 18, 2019
Merged

Conversation

frankchen07
Copy link
Contributor

@frankchen07 frankchen07 commented Dec 6, 2019

Description

Added three methods:

  1. method that translates grant dictionary data into a list of lists
  2. method that aggregates and calculates the by pair totals
  3. method that continually tests thresholds until the proper total CLR amount is found, and calculates the result

Testing

I was going through the curve calculation part, and I believe it may break it. My concern is around line 224, but it may be that it's alright. Let's pair up and take look at that @thelostone-mc.

@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #5608 into master will increase coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5608      +/-   ##
==========================================
+ Coverage   30.05%   30.09%   +0.03%     
==========================================
  Files         248      248              
  Lines       21404    21396       -8     
  Branches     3105     3108       +3     
==========================================
+ Hits         6434     6440       +6     
+ Misses      14694    14680      -14     
  Partials      276      276
Impacted Files Coverage Δ
app/grants/management/commands/estimate_clr.py 0% <0%> (ø) ⬆️
app/grants/clr.py 0% <0%> (ø) ⬆️
app/dashboard/embed.py 31.6% <0%> (+3.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a25ab67...cfc7ffb. Read the comment docs.

@owocki
Copy link
Contributor

owocki commented Dec 6, 2019

how much faster is this? have we profiled it in a production like environment?

app/grants/clr.py Outdated Show resolved Hide resolved
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

left a bunch of minor comments

app/grants/clr.py Outdated Show resolved Hide resolved
app/grants/clr.py Outdated Show resolved Hide resolved
app/grants/clr.py Show resolved Hide resolved
app/grants/clr.py Show resolved Hide resolved
app/grants/clr.py Outdated Show resolved Hide resolved
@thelostone-mc
Copy link
Member

thelostone-mc commented Dec 11, 2019

@owocki so
@frankchen07 did a run here https://github.com/thelostone-mc/clr/tree/frank-dev

Ran both the algo against the previous clr dataset
algorithm: ran 1 time
data: previous clr data set from live
speed: ~5x faster

Perks of getting this PR in:

  • this is 5x faster

Reality:

  • it's not still fast enough to be able to run it on demand and has to run has a cron
  • it can be run ~30times as opposed to ~6 times per day which makes the results a lil more accurate but not something the user will notice
  • the end results of both algo are the same

cc @gitcoinco/engineers

@thelostone-mc thelostone-mc force-pushed the frank-clr-update branch 2 times, most recently from f5b0134 to 3edef90 Compare December 16, 2019 20:37
- udpate logger to specify which grant has no contributions
- removed duplicate binary search
- grant is updated + via clr only if it has
  contributions
- reindented comments + removed print stmt
@thelostone-mc thelostone-mc merged commit 2b0ccc5 into gitcoinco:master Dec 18, 2019
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.

4 participants