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.

Sign up for GitHub

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

Jump to bottom

Fix #1048 - The accuracy module testing, fixes and readme #1057

Closed
wants to merge 4 commits into from
Closed

Fix #1048 - The accuracy module testing, fixes and readme #1057

wants to merge 4 commits into from

Conversation

kdetry
Copy link
Contributor

@kdetry kdetry commented May 16, 2024 •
edited
Loading

Fixes #1048

Changes proposed in this PR:

  • make sure the app endpoint is working as expected
  • the app needs to have a lake, that it's constantly updating the raw tables (not ETL)
  • have instructions for how to deploy this
  • have instructions for how to test that it's working
  • make sure the predictoor.ai FE still works once the new server is deployed

@kdetry kdetry changed the base branch from main to issue685-duckdb-integration May 16, 2024 13:54
@kdetry kdetry changed the title Issue 1048 Fix #1048 - The accuracy module testing and readme May 16, 2024
KatunaNorbert
KatunaNorbert reviewed May 16, 2024
View reviewed changes
pdr_backend/accuracy/readme.md
curl http://localhost:5000/statistics
```

The endpoint will return the accuracy of the model in JSON format.
Copy link
Member

Choose a reason for hiding this comment

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

We could provide a JSON example of the response with all the fields and also mention that the API returns multiple values not just accuracy(token_name, total_staked_yesterday, total_staked_today), hence the statistics names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will bring an example JSON and mention about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put the information to the "Usage" subtitle, under the json file info

@kdetry kdetry changed the title Fix #1048 - The accuracy module testing and readme Fix #1048 - The accuracy module testing, fixes and readme May 16, 2024
@kdetry kdetry requested a review from idiom-bytes May 17, 2024 10:07
KatunaNorbert
KatunaNorbert reviewed May 20, 2024
View reviewed changes
pdr_backend/accuracy/readme.md
```


The script uses the `GQLDataFactory` to fetch the predictions. Be sure to provide the correct values for `st_timestr` in the `ppss.yaml` file to get the correct predictions. It needs to be at least `28 days ago` from the current date.
Copy link
Member

Choose a reason for hiding this comment

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

Change 'to get correct predictions' to 'to get the required data for the calculations'

KatunaNorbert
KatunaNorbert reviewed May 20, 2024
View reviewed changes
pdr_backend/accuracy/readme.md

## Usage

The `app.py` script is used to calculate the accuracy of a model based on the predicted and true values.
Copy link
Member

Choose a reason for hiding this comment

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

'to calculate the accuracy' -> 'to calculate the accuracy and other statistics'

@idiom-bytes
Copy link
Member

[Accuracy Update]
@kdetry

  1. I really believe we should keep the accuracy/ app as-is in main so we can get the duckdbpr ETL to where it needs to be
  2. We can then focus on improving bronze-predictions and getting predictoor revenue complete

I would move all the latest duckdb/app code that uses lake/duckdb to another PR/branch and freeze that for later.

@idiom-bytes
Copy link
Member

idiom-bytes commented May 28, 2024 •
edited
Loading

@kdetry as far as I can tell, this PR is to "fix" the Accuracy API w/ DuckDB and get the PR ready, but....

  1. This PR does not include the original "Accuracy API"
  2. This PR does not help me conclude the work with duckdb pr and get the PR ready

Please conclude your work here and get this PR ready to go so I can ready the DuckDB PR, before beginning work on 1001..

@idiom-bytes idiom-bytes mentioned this pull request May 31, 2024
Closed
3 tasks
@idiom-bytes
Copy link
Member

We're going to close this PR.

Rather than trying to merge the whole duckdb PR, we're going to merge it in phases.
#1109

I'll be looking at this PR to potentially completing the integration.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KatunaNorbert KatunaNorbert KatunaNorbert left review comments

@idiom-bytes idiom-bytes Awaiting requested review from idiom-bytes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kdetry @idiom-bytes @KatunaNorbert