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

[Freeze][Do Not Merge] - Created silver predictions table #665

Conversation

KatunaNorbert
Copy link
Member

@KatunaNorbert KatunaNorbert commented Feb 22, 2024

Fixes #746

idiom-bytes and others added 30 commits January 19, 2024 14:17
…leaned up logic to follow latest implementations and verified tests are working.
…ta as ms. Plots are now charting as expected.
…ms timestamp, not raw_subgraph/Predictions... this cold be improved further
…mmed addresses, ms, and other details that were lost in the test configuration.
idiom-bytes and others added 5 commits February 23, 2024 17:29
…d gets composed correctly. Updated tests to handle current errors. I believe there are columns from subgraph fetch that we can drop
* the function sign has changed

* fix table test

---------

Co-authored-by: Norbert <[email protected]>
Base automatically changed from issue-495-2 to main February 26, 2024 10:40
@KatunaNorbert
Copy link
Member Author

Just did some testing agains real data, unfortunately the table creation with all the calculations take too much time for big number of data. It needs some improvements..
ex: for 4685 row it took 101 sec to compute the table

df_subscriptions: DataFrame,
user_subscriptions: DataFrame,
df_slots: DataFrame,
) -> dict:
Copy link
Member

@idiom-bytes idiom-bytes Mar 15, 2024

Choose a reason for hiding this comment

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

This looks great @KatunaNorbert!

It provides an outline for all the calculations we'll have to do for the silver table, for each individual parameter... awesome! Now that we know these, we'll be able to more easily write them in SQL.

  1. Don't worry about cleaning this up, splitting into functions, etc...
  2. Focus on improving test coverage such that you can validate that the values are calculated correctly
  3. Once you feel tests are validated well enough, let's move onto streamlit.

I have upated the Epic + streamlit task to reflect how you should move forward. Specifically, look at Calina's PRs and follow the deliverables on #612

@idiom-bytes
Copy link
Member

Hey @KatunaNorbert,

I reviewed the implementation, some of the test coverage, the PR in general, and it's shaping up really well!
I think your test coverage is pretty good, and I noticed it's validating that the revenue calculations are correct. Great!

I have provided some feedback (minor) and next steps for how to move forward.

[Final Requirement]
Right now test_etl.py is failing.

Please get the pdr_backend/lake tests passing locally (don't worry about CICD), such that you know that the ETL code is working end-to-end before starting on the streamlit/plotting work.

@kdetry kdetry changed the base branch from main to issue685-duckdb-integration March 22, 2024 15:20
@idiom-bytes idiom-bytes changed the title Created silver predictions table [Freeze][Do Not Merge] - Created silver predictions table Apr 12, 2024
@idiom-bytes idiom-bytes mentioned this pull request Jun 25, 2024
35 tasks
@KatunaNorbert KatunaNorbert deleted the issue610-create-silver-predictions-table branch June 26, 2024 06:39
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.

[Lake] Calculate df and stake revenues for silver predictions table
3 participants