Skip to content

Deciding how many significant figures to show for BigDecimal #868

New issue
New issue
Closed
Closed
Deciding how many significant figures to show for BigDecimal#868
Assignees
leoyvens
Labels
area/graphqlbugSomething isn't workingneeds discussion

Description

@davekay100
davekay100
opened on Apr 10, 2019

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
A BigDecimals can have up to 128 significant figures, such as this:
"0.000000000000000000000000004056857188988099386043441332328976217179316194185743029850020662656864699416968852342972096539937360"

What behavior do we want?

I think we made some good arguments from the start about why we want to keep a lot of precision #720

However this level of precision isn't easy on the eyes. It would be good if we could allow developers to choose how precise BigDecimal goes. For example, many solidity developers might set up their code so a token only has 8 decimal places. In this case, we don't need 128 bits of precision. The simplest solution I can think of is something like this:

let largeNumber: BigDecimal = "0.000000000000000000000000004056857188988099386043441332328976217179316194185743029850020662656864699416968852342972096539937360"

largeNumber.decimals(35)

console.log(largeNumber) --> "0.00000000000000000000000000405685718"

Basically a method that lets the developer choose . Open to other suggestions! This is only a first pass.

Activity

  1. davekay100 commented on Apr 10, 2019

    @davekay100
    davekay100
    on Apr 10, 2019
    ContributorAuthor

    Talking with @leodasvacas he suggested a way to do this, and I implemented it like so in a mapping:

    // For each division by 10, add one to exponent to truncate one significant figure
function truncateBigDecimal(bd: BigDecimal, truncateAmount: i32):BigDecimal {

  // This will shave off the actual digits, so our BigInt is getting smaller
  for(let i = 0; i < truncateAmount; i++){
    bd.digits = bd.digits.div(BigInt.fromI32(10))
  }

  // This adds to the exponent, which is negative for numbers below zero
  // and moves the decimal point to be in line with the fact that the digits BigInt got 1 length shorter
  bd.exp = bd.exp.plus(BigInt.fromI32(truncateAmount))

  return bd
}

    This works, but it is untested. It works in the case where I knew all my BigDecimals would be smaller than 1.0. I'm not sure if it would work with 234325.1241353144342342342.

    But it is a start!

  2. leoyvens
    added
    needs discussion
    area/graphql
    on Apr 10, 2019

  3. leoyvens commented on Apr 10, 2019

    @leoyvens
    leoyvens
    on Apr 10, 2019
    Contributor

    However this level of precision isn't easy on the eyes

    Looking at a query is something we do when developing, but primarily it is meant to be consumed by machines which can then round and format a number in a bunch of different ways for presentation. IMO that's mostly not our concern, it's easy for the consumer to do and we can't cover all cases anyways.

    We should however have some limit, because right now the number of zeroes we print after or before the dot can easily get very large. IMO we should then switch to exponential notation. Maybe we should always use exponential notation?

  4. davekay100 commented on Apr 10, 2019

    @davekay100
    davekay100
    on Apr 10, 2019
    ContributorAuthor

    Yeah I agree with everything you are saying.

    Can we just have a helper function, like the one above, in the graph-ts library - and let the consumer use it at their own discretion? This way we don't have to change anything, but if consumers find it too long they can shorten it themselves.

    Also for reference, Right now I believe the digits max out at 128 bits, and so that is 0 and . with 126 numbers after the decimal. I guess there can only be 127 digits combined, left or right of the period

  5. davekay100 commented on Apr 10, 2019

    @davekay100
    davekay100
    on Apr 10, 2019
    ContributorAuthor

    would exponential notation take a lot of time to implement?

  6. leoyvens commented on Apr 11, 2019

    @leoyvens
    leoyvens
    on Apr 11, 2019
    Contributor

    @davekaj By consumer I meant querier, not the developer of the subgraph. But yes having that helper wouldn't hurt. The limit shouldn't be 128, could you confirm that if you divide by 10 a bunch of times, you never get more than 128 digits? That would be a bug. Exponential notation should be easy to implement.

  7. davekay100 commented on Apr 11, 2019

    @davekay100
    davekay100
    on Apr 11, 2019
    ContributorAuthor

    Screen Shot 2019-04-11 at 12 13 31 PM

    ahhh my hypothesis was wrong apparently. you can get them really long.

    but for some reason all the data I was having returned from normal calculations never was longer than 128 digits

  8. leoyvens
    self-assigned this
    on Apr 11, 2019
  9. leoyvens
    added
    bugSomething isn't working
    on Apr 11, 2019

  10. davekay100 commented on Apr 15, 2019

    @davekay100
    davekay100
    on Apr 15, 2019
    ContributorAuthor

    This new function I created works very well, you just pass it how many decimals you desire.

    I think if we add this helper to graph-ts, it might solve this issue completely.

    // Must keep all values left of the decimal, and then allow user to decide how many decimals they want
export function truncateBigDecimal(bd: BigDecimal, decimalLength: i32): BigDecimal {
  
  // Figuring out how many digits to truncate
  // exp is negative if there are decimals, so we add it, unintuitively 
  let largerThanZeroLength = bd.digits.toString().length + bd.exp.toI32() 
  
  // if largerThanZeroLength is negative, then digit length will be less than 18, which is okay
  let newDigitLength = decimalLength + largerThanZeroLength
  let lengthToTruncate = bd.digits.toString().length - newDigitLength

  // This means it was originally smaller than desired decimalLength, so do nothing
  if (lengthToTruncate < 0) {
    return bd
  } else {
    // This will shave off the length of the full digits to what is desired
    for (let i = 0; i < lengthToTruncate; i++) {
      bd.digits = bd.digits.div(BigInt.fromI32(10))
    }
    // simply set the exp to what was desired (* -1 because it must be negative, but as parameter it is passed in postive
    bd.exp = BigInt.fromI32(decimalLength* -1)
    return bd
  }
}

  11. Jannis commented on Apr 15, 2019

    @Jannis
    Jannis
    on Apr 15, 2019
    Contributor

    I'd like us to not drop more stuff into the helpers file. We need to be deliberate about the APIs that we expose and support going forward.

    But I could see us adding a function like this to BigDecimal. @leodasvacas: What do you think?

  12. leoyvens commented on Apr 15, 2019

    @leoyvens
    leoyvens
    on Apr 15, 2019
    Contributor

    I don't see a problem in adding that helper as a method on BigDecimal, if @davekaj is willing to make a PR to graph-ts.

  13. davekay100 commented on Apr 15, 2019

    @davekay100
    davekay100
    on Apr 15, 2019 via email
    ContributorAuthor

  14. leoyvens commented on Apr 15, 2019

    @leoyvens
    leoyvens
    on Apr 15, 2019
    Contributor

    I want to set up a testing infrastructure in graph-ts that compiles test files to wasm and runs them, but right now no, there isn't.

  15. davekay100 commented on May 17, 2019

    @davekay100
    davekay100
    on May 17, 2019
    ContributorAuthor

    I think we can close this now that truncate() is in graph-ts

  16. davekay100
    closed this as completedon May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area/graphqlbugSomething isn't workingneeds discussion

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions