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

Fix #1384: Improve Predictoors Table #1398

Merged
merged 11 commits into from
Jul 17, 2024
Merged

Conversation

kdetry
Copy link
Contributor

@kdetry kdetry commented Jul 15, 2024

Fixes #1384

Changes on this PR:

  • the total_profit, total_acc, avg_stake columns have been added.
  • the sorting logic has been added
  • the user column is shortened

@calina-c
Copy link
Contributor

calina-c commented Jul 16, 2024

@kdetry @KatunaNorbert @idiom-bytes @trentmc
Looking at this PR and working with the predictoor dashboard (and honestly using my personal banking app) recently have given me an idea. At most times, people will only be interested in some specific accounts and it's difficult for them to manage the addresses of those accounts. I propose we add an aliasing table, where users can locally "name" their accounts in a user-friendly way or maybe mark them as favourites (and show them on top of the list). We can possibly do some sort of "favourite" mark for feeds, as well.

Improvements I see:

  • easier to filter and manage. The user can set the name for "0xabc1234..." as "Alice", "0xefgh5678..." as "Bob" and filter by those values. We can even toggle smth like:
    • only showing the favourites
    • showing the favourites on top
    • showing everything
      obviously the first case would ease a lot of mental load for the user and database load for the app itself
  • maybe autoselection based on favourites?
  • this goes for feeds too, if we let the user mark some feeds of interest and show them on top/exclusively

For us it is an extra JOIN, but for the user it might be a huge time-saver since they don't have to paste in filters all the time.

Let me know what you guys think. Maybe it's a bad idea, maybe it's not doable now, but I'd like to discuss it and have an issue if it looks useful to you.

@trentmc
Copy link
Member

trentmc commented Jul 16, 2024

people will only be interested in some specific accounts and it's difficult for them to manage the addresses of those accounts. I propose we add an aliasing table, where users can locally "name" their accounts in a user-friendly way

Gm @calina-c , you're absolutely right, people mostly want to see their accounts, or compare their accounts to global.

And in fact, @KatunaNorbert and I were talking about it yesterday:)

And, we discussed a simple way to do it: specify via ppss.yaml.


My specific suggestion: have a new section in predictoor_ss called "my_addresses" where you can optionally list your addresses. That's it.


Then this can get picked up by Predictoor Dashboard or anywhere else.

  • We know how to deal with address format already, see eg web3_pp section.
  • I'm recommending to put it into predictoor_ss, versus more general, because we may want to specify different addresses for other contexts (eg traders). One step at a time.

Appendix: The Predictoor Dashboard prototype slides had this feature too:
Screenshot 2024-07-16 at 09 28 19

@KatunaNorbert
Copy link
Member

KatunaNorbert commented Jul 16, 2024

And in fact, @KatunaNorbert and I were talking about it yesterday:)

I like it when multiple people have the same idea, this means it's something of a high value.

We can prioritise it, add it to v0.2 and start working on it right away. Looks like we have a clear description on how to implement it:

  • specify addresses inside ppss.yaml
  • make addresses and corresponding feeds selected inside the table when the app starts
  • nice to have: add a button so app resets to your predictoors when clicking on a button ex: 'My Predictoors'

@trentmc
Copy link
Member

trentmc commented Jul 16, 2024

Looks like we have a clear description on how to implement it:
specify addresses inside ppss.yaml

Yes, and specifically, put it inside the predictoor_ss section. Because it's really for people running predictoors. This also allows us to leverage all the predictoor_ss infra.

@KatunaNorbert
Copy link
Member

Created the issue:
#1402

@kdetry
Copy link
Contributor Author

kdetry commented Jul 16, 2024

I found it inconvenient to fetch all payouts and calculate the table stats with Python because of reloading a huge data on memory. Instead, I implemented an SQL logic to improve performance and reduce performance costs.

https://github.com/oceanprotocol/pdr-backend/pull/1398/files#diff-0e2d99ab25f98454e058b613399431ead8e2af3aa527ba7a6b6ddeca7af610ceR57

@kdetry kdetry requested review from KatunaNorbert and calina-c July 16, 2024 13:46

result = get_user_payouts_stats_from_db(ppss.lake_ss.lake_dir)

print("result--->", result)
Copy link
Member

Choose a reason for hiding this comment

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

remove print or use logger if needed

@@ -142,13 +145,49 @@ def update_predictoors_table_on_search(
predictoors_data, "user", search_value, selected_predictoors
)

if user_payout_stats:
# Her bir data elemanı için, eşleşen user_payout_stat bulunur ve güncellenir.
# Eşleşme yoksa, orijinal data elemanı korunur.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should translate this to english 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my fault :D

"avg_accuracy": round(data["avg_accuracy"], 2),
"avg_stake": round(data["avg_stake"], 2),
"user": data["user"],
}
Copy link
Member

@KatunaNorbert KatunaNorbert Jul 17, 2024

Choose a reason for hiding this comment

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

can we save this values directly on the predictoor_data state inside get_input_data_from_db so we don't have to filter and select them on each predictoor table action?

GROUP BY
"user"
""",
)
Copy link
Member

Choose a reason for hiding this comment

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

good job with the query, the data comes in very fast!
I'm wondering if we can unite this query with the get predictoors one and have a get predictoors data query containing all the data we need, should be straight forward.

@KatunaNorbert
Copy link
Member

Could you also make make the column containing the tables wider? It should be wide enough to have all the columns visible



@enforce_types
def process_user_payout_stats(
Copy link
Member

Choose a reason for hiding this comment

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

Looks much cleaner now, but we should add a better name to this function: "get_predictoors_data_from_payouts" or something similar

@KatunaNorbert
Copy link
Member

The accuracy and avg stake on the predictoor row doesn't mach the values from the metrics. I have all the feeds selected for the predictoor so they should mach:
Screenshot 2024-07-17 at 14 43 05

@kdetry
Copy link
Contributor Author

kdetry commented Jul 17, 2024

The accuracy and avg stake on the predictoor row doesn't mach the values from the metrics. I have all the feeds selected for the predictoor so they should mach: Screenshot 2024-07-17 at 14 43 05

for predictor, feed in product(predictoors, feeds):
        # only filter for this particular predictoor and feed pair
        # in order to properly group the data
        filtered_payouts = [
            p for p in payouts if predictor in p["ID"] and feed.contract in p["ID"]
        ]

        slots, accuracies, profits, stakes = process_payouts(filtered_payouts)

        if not slots:
            continue

        avg_stake = ((stakes[-1] + avg_stake) / 2) if avg_stake else stakes[-1]
        total_profit = (profits[-1] + total_profit) if total_profit else profits[-1]
        avg_accuracy = (
            ((accuracies[-1] + avg_accuracy) / 2) if avg_accuracy else accuracies[-1]
        )

In the code block above, you averaged the accuracy value calculated for each pair and predictor. In my example, I calculate the average of all payouts of the predictor.

@KatunaNorbert
Copy link
Member

I see, so it should be fixed in the metrics calculation then

Copy link
Member

@KatunaNorbert KatunaNorbert left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for also fixing the metrics issue

@kdetry kdetry merged commit 32d2f24 into issue-1357-PdrDashboard-v02 Jul 17, 2024
4 checks passed
@kdetry kdetry deleted the issue-1384 branch July 17, 2024 14:01
KatunaNorbert added a commit that referenced this pull request Jul 22, 2024
* Fix #1376: Change callbacks to not store payout data (#1378)

* Fix #1375: Add Buttons and Sorting (#1379)

* Fix #1373: Separate some concerns in pdr-dashboard. (#1390)

* Fix #1374: Display metrics over multiple feeds and predictoors (#1377)

* Fix #1400: Fix selections in feed and predictoor list (#1401)

* Fix #1385: Add switch to display selected predictoors feeds only (#1395)

* Fix #1402: Allow address config for predictoor dashboard. (#1403)

* Fix #1404: Allow date period selection (#1405)

* Fix #1384: Improve predictoors table by adding accuracy, stake and profit (#1398)

* Fix #1409: Add predictoor and feed startup data instead of loading on a dash component (#1411)

* Fix #1412:  Update dashboard readme with new instructions (#1414)

* Fix #1413: Update and split dash_duo tests (#1417)
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