-
Notifications
You must be signed in to change notification settings - Fork 54
Add Rawpixel to image popularity recalculation logic #897
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.
The new test is great!
("0007_openledger_audio_view.sql", sql.AUDIO_POPULARITY_METRICS), | ||
], | ||
) | ||
def test_ddl_matches_definitions(ddl_filename, metrics): |
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.
So cool!
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.
Awesome!
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.
Thanks! I always think of adding it to the SQL file but I didn't even remember the logic file existed 😅
@@ -9,7 +9,8 @@ INSERT INTO public.audio_popularity_metrics ( | |||
provider, metric, percentile | |||
) VALUES | |||
('wikimedia_audio', 'global_usage_count', 0.85), | |||
('jamendo', 'listens', 0.85); | |||
('jamendo', 'listens', 0.85), | |||
('freesound', 'num_downloads', 0.85); |
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.
Nice catch! Perhapsfreesound
should be included in the title too?
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.
This is just the SQL DDL, which is only used locally. Since Freesound was present in logic file, it was added (either manually or automatically) to the appropriate table quite a while ago. Confirmed in production:
deploy@localhost:openledger> select * from audio_popularity_metrics;
+-----------------+--------------------+------------+
| provider | metric | percentile |
|-----------------+--------------------+------------|
| jamendo | listens | 0.85 |
| wikimedia_audio | global_usage_count | 0.85 |
| freesound | num_downloads | 0.85 |
+-----------------+--------------------+------------+
SELECT 3
Time: 0.098s
So this is more a bookkeeping change than anything 😄
("0007_openledger_audio_view.sql", sql.AUDIO_POPULARITY_METRICS), | ||
], | ||
) | ||
def test_ddl_matches_definitions(ddl_filename, metrics): |
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.
Awesome!
Description
This is a follow up to #795 which adds the Rawpixel popularity metric to the python logic steps. It was added to the DDL as part of #795, but it was not added to the table itself and said table gets updated in Python as part of the image popularity constant recalculation step once monthly. Even if it was added directly to the table, it wouldn't get updated if it were adjusted later down the line.
I also added a test which ensures that each of the values defined in the python logic are present in the DDL, and vice versa. This actually identified that Freesound was missing from the DDL, which I added in this PR.
We can either add Rawpixel's metric to the table directly after this, or we can wait until the next full popularity recalculation.
Testing Instructions
Tests should be sufficient!
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin