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

Price bucket rounding error. #2350

Closed
mkendall07 opened this issue Apr 4, 2018 · 6 comments
Closed

Price bucket rounding error. #2350

mkendall07 opened this issue Apr 4, 2018 · 6 comments
Assignees
Labels

Comments

@mkendall07
Copy link
Member

mkendall07 commented Apr 4, 2018

Type of issue

bug

Description

When using this price granularity:

    const customGranularity = {
      'buckets': [{
          'min': 0,
          'max': 10,
          'increment': 0.01
      }]
    };

A bid price of 4.01000 is rounded down to 4.00. It should be rounded to 4.01

@stale
Copy link

stale bot commented Apr 18, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 18, 2018
@mkendall07 mkendall07 added bug and removed stale labels Apr 19, 2018
@lafentres
Copy link

lafentres commented May 10, 2018

I am also experiencing an issue with rounding when using custom price granularity. My price buckets are set up like this:

var customPriceBuckets = {
        'buckets': [
            // 1 cent increments up to 50 cents
            {
                'min': 0.00,
                'max': 0.50,
                'increment': 0.01
            },
            // 5 cent increments up to 5 dollars
            {
                'min': 0.50,
                'max': 5.00,
                'increment': 0.05
            },
            // 10 cent increments up to 10 dollars
            {
                'min': 5.00,
                'max': 10.00,
                'increment': 0.10
            },
            // 50 cent increments up to 20 dollars
            {
                'min': 10.00,
                'max': 20.00,
                'increment': 0.50
            },
            // 1 dollar increments up to 50 dollars
            {
                'min': 20.00,
                'max': 50.00,
                'increment': 1.00
            }
        ]
    };

I'm seeing a bid of 2.55 come thorough as the cpm but my hb_pb value is getting set to 2.50. It should be 2.55, based on my custom price granularity. The raw bid in the response is actually 2.550000
screen shot 2018-05-10 at 10 38 30 am

@lafentres
Copy link

lafentres commented May 15, 2018

I'm currently trying to see if I can figure out why this is happening.

Adding 2.550000 to the cpmInputs and {"low":"2.50","med":"2.50","high":"2.55","auto":"2.55","dense":"2.55","custom":""} to the priceStringOutputs in the cpmInputsOutputs.json file causes the "getPriceBucketString function generates the correct price strings" test to fail.

Probably obvious, but this is the line of code where things are going wrong:
let cpmTarget = ((Math.floor((cpm - bucketMin) / increment)) * increment) + bucketMin;

I've tried a couple different formulas and they get close but still aren't quite right and cause some failing tests. Here's what I've tried:
let cpmTarget = increment * (Math.round(cpm/increment));
let cpmTarget = cpm + (increment/2) - (cpm + (increment/2)) % increment;

@jsnellbaker jsnellbaker self-assigned this May 16, 2018
@jsnellbaker
Copy link
Collaborator

From investigating further, the issue appears to be how dividing with decimals is handed in JavaScript. For example, when you divide 4.01 / 0.01 in a calculator you would get 401. However if you do the same thing in JavaScript (easily see this in the browser console), you get 400.99999999999994. There are numerous sets of values that result in this manner, either slightly higher or lower than the expected mark (eg 1.11 / .01 = 111.00000000000001).

When the Math.floor gets applied to these low rogue values, it'll reduce it down one level below it's expected value. So 400.99999999999994 becomes 400, which is how we ultimately see a 4.01 cpm end up in the 4.00 cpm bucket when we have 0.01 increments.

I've found an adjustment to the formula that appears to work, by doing a precision rounding to 3 decimal places on the quotient before applying the floor. The precision rounding logic I plan to use was found here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round#A_better_solution This modifies the value to the expected state for the rest of the formula to place the cpm in the ideal bucket.

I'm doing some further analysis on various cpms/increment values to see if it's consistently working, as well as checking the results when using the standardized buckets (ie low, medium, etc) since they use the same formula for the cpm assignment.

@jsnellbaker
Copy link
Collaborator

Update on the previously noted solution.

Based on further analysis, we've decided to implement a different solution that was found to be more efficient performance-wise.

We will multiply the individual values of the formula that gets floored (ie (cpm - bucketPrice) / increment by an exponential base of 10 (eg 100000) to avoid doing the math with decimals which subsequently avoids any rounding issues.

see https://jsperf.com/cpm-bucket-rounding/ to the unit test comparison.

@mkendall07
Copy link
Member Author

resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants