toBigInt should round identically for Double and BigDecimal #3921
Conversation
| @@ -59,6 +59,20 @@ class LiteralExtractorSpec extends ChiselFlatSpec { | |||
| bigIntFromDouble should be(bigInt53) | |||
| } | |||
|
|
|||
| "doubles and big decimals" should "be rounded identically" in { | |||
|
|
|||
| for (double <- Seq(1.0, 1.1, 1.5, 1.6, 2.5)) { | |||
There was a problem hiding this comment.
This has both 1.5 and 2.5 which means that this is checking to make sure it's not using HALF_EVEN. 👍
Co-authored-by: Schuyler Eldridge <[email protected]>
| @@ -205,9 +206,9 @@ trait NumObject { | |||
| * @param binaryPoint a binaryPoint that you would like to use | |||
| * @return | |||
| */ | |||
| def toBigInt(x: BigDecimal, binaryPoint: Int): BigInt = { | |||
| def toBigInt(x: BigDecimal, binaryPoint: Int, roundingMode: RoundingMode = HALF_UP): BigInt = { | |||
There was a problem hiding this comment.
Since this is a bugfix, I'd like to backport it. Stable branches check binary compatibility and obnoxiously in Scala (2 at least, not sure about 3) it is not binary compatible to add an argument with a default parameter--instead you have to just overload the method, i.e.:
def toBigInt(x: BigDecimal): BigInt = toBigInt(x, HALF_UP)
def toBigInt(x: BigDecimal, binaryPoint: Int, roundingMode: RoundingMode): BigInt = ...I will push this change to your branch, I just wanted to note it before we merge.
There was a problem hiding this comment.
~I just realized the version without the default argument already exists, I'm surprised this isn't causing a problem... ~, nevermind, that's the version taking Double, got it.
--------- Co-authored-by: Schuyler Eldridge <[email protected]> Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit b9f5703) # Conflicts: # core/src/main/scala/chisel3/Num.scala # src/test/scala/chiselTests/LiteralExtractorSpec.scala
--------- Co-authored-by: Schuyler Eldridge <[email protected]> Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit b9f5703)
--------- Co-authored-by: Schuyler Eldridge <[email protected]> Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit b9f5703)
…3926) --------- Co-authored-by: Schuyler Eldridge <[email protected]> Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit b9f5703) Co-authored-by: Michiel Van Beirendonck <[email protected]>
…3927) --------- Co-authored-by: Schuyler Eldridge <[email protected]> Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit b9f5703) Co-authored-by: Michiel Van Beirendonck <[email protected]>
|
@mvanbeirendonck would you like a release with this fix in the near term? What version of Chisel are you using? |
Not very urgent! Just wanted to report this one. I am still on Chisel 3.5.6. |
|
@mvanbeirendonck fix released in Chisel v6.3.0 and v5.2.0, hopefully you can get there from Chisel 3.5 😅. This will also be in v3.6.1 when I eventually get to it. |
This very minor PR fixes a bug in
Num.toBigInt(x: BigDecimal, binaryPoint: Int).The current function gives the impression to round the input BigDecimal:
However:
I'm assuming this is not the intended behaviour.
The underlying issue is with the
roundedfunction which uses the BigDecimal'sMathContext. I changed this to usesetScale(0)instead.I added a testcase for rounding positive numbers. I would've added a testcase for negative ones, but as far as I know there is no
RoundingModethat replicates the rounding of Doubles there (round half to positive infinity).Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.