<< Back to previous view

[CLJ-1118] inconsistent numeric comparison semantics between BigDecimal and other Numerics Created: 30/Nov/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Arthur Ulfeldt Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: math

Attachments: Text File clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt     Text File clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt     Text File clj-1118-v6.txt     Text File clj-1118-v7.patch    
Patch: Code and Test
Approval: Ok

 Description   

BigDecimal does not have consistent comparison semantics with other numeric types.

user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false                  <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
user> (== 1.0M 1.00M)
false    ;; potentially surprising

Patch: clj-1118-v7.patch

Approach: Change equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The javadoc for these methods explicitly states that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies hasheq() to return the same value for all numerically equal BigDecimal values, by calling BigDecimal.stripTrailingZeros() on them before hashing. This change also will make 1.0M == 1.00M which was not true before.

Screened by: Alex Miller



 Comments   
Comment by Arthur Ulfeldt [ 30/Nov/12 1:51 PM ]

I understand that the definition of equality between bigDecimals is dependent on both value and scale as in this case:

user> (== 0.000000M 0.0M)
false

I just want to make sure the decission to propagate that semantic across types is intentional. If this is on purpose than this is not a bug.

Comment by Arthur Ulfeldt [ 30/Nov/12 2:03 PM ]

this could be fixed by calling stripTrailingZeros on bigDecimals before comparing them to Longs or BigInts.

(== 2 (double (. 2.0M stripTrailingZeros)))
true

Edited by Andy Fingerhut: Unfortunately that fails for BigDecimal values equal to 0, unless they happen to have a scale that matches what you are comparing it to.

I think a more complete solution is to use BigDecimal's compareTo method, e.g.:

(zero? (.compareTo 2.0M (bigdec 2)))
true

Comment by Timothy Baldridge [ 03/Dec/12 11:31 AM ]

It seems we need some more eyes on this issue, can you bring this up on clojure-dev and see what they think?

Comment by Andy Fingerhut [ 14/Apr/13 4:03 AM ]

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt dated Apr 14 2013 changes equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The Java docs for these methods explicitly state that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal.

They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

I'm not sure if this is the preferred behavior for Clojure, but if it is, this patch should do it.

Comment by Andy Fingerhut [ 15/Apr/13 12:18 AM ]

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt dated Apr 14 2013 is same as clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt described in previous comment, except it also has some new tests included.

Comment by Andy Fingerhut [ 15/Apr/13 9:07 PM ]

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v3.txt dated Apr 15 2013 is the same as the the previous patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt, except for the following:

By changing == behavior for BigDecimal by modifying the BigDecimalOps.equiv() method, that also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return same value for all numerically equal BigDecimal values. This patch should achieve that.

Comment by Andy Fingerhut [ 14/Aug/13 7:29 PM ]

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt dated Apr 15 2013 is simply updating the stale -v3.txt patch. The only reason it no longer applied cleanly with git is because a few lines of context changed in the test code.

Comment by Andy Fingerhut [ 27/Sep/13 11:55 AM ]

Quick comment that some of the tests in patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt were written before CLJ-1036 was declined as out of scope for Clojure, and should probably be cut down a bit, at least to eliminate biginteger and float occurrences. I will wait and see if there are any other comments from screening before creating any new patches.

Comment by Alex Miller [ 27/Sep/13 12:21 PM ]

Andy, the tests in the patch don't actually pass for me?

Many failures in the generative tests, but for example:

[java] {:clojure.test/vars (equality-tests),
     [java]  :thread/name "main",
     [java]  :pid 5719,
     [java]  :thread 1,
     [java]  :type :assert/fail,
     [java]  :message
     [java]  "Test that 0 (class java.lang.Byte) #'clojure.core/== 0.0 (class java.math.BigDecimal)",
     [java]  :level :warn,
     [java]  :test/actual false,
     [java]  :test/expected (equal-var val1 val2),
     [java]  :line 58,
     [java]  :tstamp 1380298603830,
     [java]  :file "numbers.clj"}
Comment by Andy Fingerhut [ 27/Sep/13 12:26 PM ]

Could you tell me the OS, version, and JDK version, and the command you used to run this test? The reason I ask is that this patch has been tested on 3 OS/JDK version combos via my prescreen testing, using 'ant', and it passes on all of them, so if it fails on some OS/JDK I am not testing I would like to figure out why.

Comment by Alex Miller [ 27/Sep/13 12:42 PM ]

Sorry - user error. They're passing.

Comment by Alex Miller [ 27/Sep/13 3:12 PM ]

Slightly tweaked patch to remove some of the float/double stuff that I'm not expecting to change.

Marking screened...

Comment by Andy Fingerhut [ 27/Sep/13 7:54 PM ]

Patch clj-1118-v6.txt is tiny modification of Alex Miller's clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt

It removes hash consistency tests for floats and BigIntegers, with a comment explaining why they should not be there.

Comment by Rich Hickey [ 22/Oct/13 7:55 AM ]

Please don't use .txt as an extension for diffs, it keeps them from loading with appropriate mode in various editors.

v5 was screened but v6 alters it?

Comment by Alex Miller [ 22/Oct/13 8:01 AM ]

Andy's last patch just added a comment. I updated a new version that is identical to v6 but has a .patch extension instead. Marking Screened again.

Comment by Andy Fingerhut [ 22/Oct/13 8:37 AM ]

It was not only adding a comment, but removing tests that should not have been there and could potentially mislead people into thinking that Clojure guarantees hash being consistent with = for BigInteger and Float/Double, which by CLJ-1036 it does not.

Regarding the .txt file extensions, first I've heard of the preference. I can make sure future screened tickets don't have this, but changing currently screened tickets would likely lead to confusion about which ones are screened. M-x diff-mode in emacs will force the mode regardless of file name.

Comment by Alex Miller [ 22/Oct/13 8:53 AM ]

Oh right, that is ok so can stay screened.

Generated at Thu Oct 30 14:04:53 CDT 2014 using JIRA 4.4#649-r158309.