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

Changed stamp karma to proper(ish) PageRank. #213

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tayler6000
Copy link
Collaborator

Changed log_error and log_exception to not be async, made log_error_async and log_exception_async instead. Old names now get the event loop for you and await the async versions for you. Changed some logging messages.
Added a recalculatestamps command which can be used for debugging, otherwise it's pretty much useless. Added a get_total_drains utility function to see how many drains there are in the PageRank calculation.

Waiting for Rob's review before merge.

(ish) comes from the fact that we're not supposed to do proper PageRank.

@tayler6000 tayler6000 added the bug Something isn't working label Feb 19, 2023
Changed log_error and log_exception to not be async, made log_error_async and log_exception_async instead.  Old names now get the event loop for you and await the async versions for you.
Changed some logging messages.
Added a recalculatestamps command which can be used for debugging, otherwise it's pretty much useless.
Added a get_total_drains utility function to see how many drains there are in the PageRank calculation.
Copy link
Collaborator

@Aprillion Aprillion left a comment

Choose a reason for hiding this comment

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

(minor) would be nicer to split the unrelated changes into 2 commits instead of just 1 commit for both log_error changes and karma changes..

modules/stampcollection.py Outdated Show resolved Hide resolved
modules/stampcollection.py Outdated Show resolved Hide resolved
modules/stampcollection.py Outdated Show resolved Hide resolved
modules/stampcollection.py Outdated Show resolved Hide resolved
modules/stampcollection.py Outdated Show resolved Hide resolved
@tayler6000
Copy link
Collaborator Author

(minor) would be nicer to split the unrelated changes into 2 commits instead of just 1 commit for both log_error changes and karma changes..

Yeah, I thought about it, but I did the karma first then realized I made the log error system in a stupid way when I went to implement it.

@tayler6000 tayler6000 self-assigned this Feb 19, 2023
@tayler6000
Copy link
Collaborator Author

I think... GitHub broke my PR...

@Gurkenglas
Copy link
Collaborator

Gurkenglas commented Feb 26, 2023

Without yet putting on my math hat or opening up VSCode - I imagine a professional code reviewer might say:

  • The SQL query is formatted like a newspaper column.
  • in this day and age and week of the year you ought to be able to tell ChatGPT to clean up the commit structure
  • Use an IDE that tells you that you deleted the usage of gamma but not its definition.
  • Take the commented-out logging code out. If someone finds some unstamply reason to do their debugging with printf, Copilot can tell them how to do a printf.
  • What problem does swapping out the algorithm solve?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants