-
Notifications
You must be signed in to change notification settings - Fork 26
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
refactor(verbosity): better handling for verbosity #109
refactor(verbosity): better handling for verbosity #109
Conversation
chore: merge development into main
Merge develop into main
feat: split reward between multiple assignees
Merge develop into main
Merge develop into main
…please--branches--main chore(main): release 1.3.0
Merge development into main
feat: using latest ChatGpt version and fixed truncated results
…please--branches--main chore(main): release 1.3.1
Merge develop into main
…ment Merge development into main
…-please--branches--main chore(main): release 1.4.0
@cohow Thank you for the PR. I am fine with the changes, please fix the Jest tests that all broke due to the results changing with the formula. |
I was looking at the tests and it seems like the problem is coming from the results not equaling the mock results and was wondering if there was a way to generate a new mock results file without having to edit all the rewards separately, if not then I'll just manage to figure something out. |
@cohow When I want to update them I copy paste the diff of the proper result so I don't have to manually fix all the numbers. However according to your screenshot there are lots of decimals to the results. I don't know if that's what we want. |
I believe these changes should fix all the issues with the mock results being incorrect. I've also changed the calculation to cut off at 2 decimal places because exponents tend to produce a lot of decimals. If you want I can also change it to round up or down, but that would require all the tests to be changed again. let me know if anything goes wrong or any expected results appear, or you require any changes, I'll try to get a fix out ASAP. |
for (const symbol of Object.keys(curr.symbols)) { | ||
const count = new Decimal(curr.symbols[symbol].count); | ||
const symbolMultiplier = new Decimal(curr.symbols[symbol].multiplier); | ||
const formattingElementScore = new Decimal(curr.score); | ||
const exponent = this._wordCountExponent; | ||
|
||
sum = sum.add( | ||
count | ||
.pow(exponent) // (count^exponent) | ||
.mul(symbolMultiplier) // symbol multiplier | ||
.mul(formattingElementScore) // comment type multiplier | ||
.mul(multiplierFactor.multiplier) // formatting element score | ||
); |
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.
word count exponent is being applied to all symbols, either we leave it like that but we should rename it to symbol exponent, or we can limit it to only words @0x4007
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.
I'm not quite sure but I feel like word count specifically makes the most sense.
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 problem is that config doesn't define word multiplier anymore, instead symbol regexes are used and technically two regexes which are a little bit different can both represent words so it's hard to apply the exponent only to the word count
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.
I had a proposal for "segments" which would serve as aliases for word regex, sentence regex, paragraph regex, entire comment regex. Perhaps we can rely on that as the main "word counter."
Ultimately I think we should strive to make the config user friendly/idiot proof but I think its impossible to cover every situation for abuse proactively. I think we should define "best practices" and hope that partners don't shoot themselves in the foot with conflicting/bad configs.
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.
I agree with having segments as aliases, we can keep regexes but I don't think many partners will bother to use that
Use a numbers library like bignumber to handle this. |
bignumber to handle the rounding or cutting off decimals? right now decimal.js is able to handle both actually so I'm not sure if another library would be needed. |
Sure use decimal for all calculations. |
if I'm not wrong the last checks tests depend on #108 or one of the issues that were linked in it. LMK if you need anything else! |
@cohow You mean that your pull-request test fixes depend on another pull-request? |
After checking the logs, it seems that everything works as expected but since your changes also modified the results the permit urls got changed as well (for example check https://github.com/ubiquibot/conversation-rewards/actions/runs/10753678522/job/29832632935?pr=109#step:4:432) so you should just have to fix the result and every test should pass. |
Ok that does make sense considering mock results are pre made.. I'm not sure why but I thought permits were being generated on the spot. I'll push changes to fix those issues when when I'm back from my class. |
@cohow Seems that the tests are still failing. I would advise running them locally or on your own repo to avoid having to wait for me revalidating workflows every time. |
ok I'm really shocked how many attempts this took for me to fix, but I finally fixed the tests I believe, tested locally and they passed. |
Seems like the permit URLs have changed, i’ll get a fix out asap. |
Quite lost on why I keep getting 2 different test results between when I run it locally and Github actions for example https://github.com/ubiquibot/conversation-rewards/actions/runs/10832843625/job/30060543240#step:4:580 Its expecting a 1.57 but receives 1.232 and when I change them I get the exact opposite "relevance": 1,
- "reward": 1.232,
+ "reward": 1.57, |
This is probably because you are using your own credentials so the status you see for the other user differs from when Ubiquibot checks the author association, changing the reward results. Also, be aware that there are two results: one comes from the JSON and one comes from the HTML file. |
@cohow Fixed the tests for you. also, seems to work, here is my QA: https://github.com/Meniole/conversation-rewards/issues/12#issuecomment-2346757261 |
Not sure if its easy to tell to be honest. How is somebody expected to manually count all their words and complain that there is a discrepancy? We can add a small blurb in the details table if somebody complains. |
Maybe something very simple like adding |
I dont think its something people will notice. We can add if its a problem. I'm just concerned it will lead to more confusion. And if we add too much info it will look bad. |
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.
If the display is fine as it is then I am good with the code changes.
Resolves #94
Reward is now calculated based on
(count^expo) * multiplier * score * multiplierFactor.multiplier
for example, im going to use issue #95
reward calculation was the following
the new verbosity algorithm would take
((65^0.85) * 0.1 * 1 * 3) + ((16^0.85) * 0.1 * 0 * 3)
which would equal to10.42
instead of the old19.5
Please check and let me know if there's any issues, also I not sure if it's an issue but I had to create a different
_calculateFormattingTotal
for calculations due to cognitive complexity related issues from lint