ubiquity-os-marketplace / text-conversation-rewards

1 stars 27 forks source link

refactor(verbosity): better handling for verbosity #109

Closed cohow closed 2 months ago

cohow commented 2 months ago

Resolves #94

Reward is now calculated based on (count^expo) * multiplier * score * multiplierFactor.multiplier

for example, im going to use issue https://github.com/ubiquibot/conversation-rewards/issues/95 reward calculation was the following

image

the new verbosity algorithm would take ((65^0.85) * 0.1 * 1 * 3) + ((16^0.85) * 0.1 * 0 * 3) which would equal to 10.42 instead of the old 19.5 image

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

gentlementlegen commented 2 months ago

@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.

cohow commented 2 months ago

@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.

image

gentlementlegen commented 2 months ago

@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.

cohow commented 2 months ago

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.

0x4007 commented 2 months ago

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.

Use a numbers library like bignumber to handle this.

cohow commented 2 months ago

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.

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.

0x4007 commented 2 months ago

Sure use decimal for all calculations.

cohow commented 2 months ago

if I'm not wrong the last checks tests depend on https://github.com/ubiquibot/conversation-rewards/pull/108 or one of the issues that were linked in it. LMK if you need anything else!

gentlementlegen commented 2 months ago

@cohow You mean that your pull-request test fixes depend on another pull-request?

cohow commented 2 months ago

@cohow You mean that your pull-request test fixes depend on another pull-request?

The last tests failed due issues with permit generation, which is not caused by this PR and after checking other PRS I believe PR #108 fixes that issue if I'm not wrong

gentlementlegen commented 2 months ago

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.

cohow commented 2 months ago

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.

gentlementlegen commented 2 months ago

@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.

cohow commented 2 months ago

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.

cohow commented 2 months ago

Seems like the permit URLs have changed, i’ll get a fix out asap.

cohow commented 2 months ago

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,
gentlementlegen commented 2 months ago

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.

gentlementlegen commented 2 months ago

@cohow Fixed the tests for you. also, seems to work, here is my QA: https://github.com/Meniole/conversation-rewards/issues/12#issuecomment-2346757261 However I think some explanation should be added to the result so the user understand what's going on in the results. @0x4007 rfc

0x4007 commented 2 months ago

@cohow Fixed the tests for you. also, seems to work, here is my QA: Meniole#12 (comment) However I think some explanation should be added to the result so the user understand what's going on in the results. @0x4007 rfc

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.

gentlementlegen commented 2 months ago

Maybe something very simple like adding coeff: 0.85 in the output? That would also show what value was used from the configuration to avoid bad surprises.

0x4007 commented 2 months ago

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.