bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Wolfgang Jenkner]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

Start Calc (M-x calc).

Try to convert -1m to cm.  Type

'-1m<return>uc

Then Calc asks

`(The expression is unitless when simplified) Old Units:'

This is the first glitch here, but, regardless, answer the question

m<return>

Now, it asks (correctly) for new units, so type

cm<return>

This gives the surprising result `-100 m' on top of the stack.

Now, if you do the same thing with -2 instead of -1, everything works fine.

The reason is that some of the functions in calc-units.el which, in
various ways, test for the presence of units in a given (simplified)
expression forget to handle subexpressions of the form (neg <expr>).

I propose the attached patch, which also contains some modest additions
to test/automated/cl-lib-tests.el.

There's also some math-defsimplify stuff in calc-units.el which fails to
handle the neg operator, but that shouldn't give rise to mathematical
errors and will be done in another patch.

This patch is also needed for (and, in fact, was prompted by) bug#19401.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: neg operator glitches --]
[-- Type: text/x-diff, Size: 6206 bytes --]

From 3b8d9b69374036a353ddfa53733c58da2c799c1a Mon Sep 17 00:00:00 2001
From: Wolfgang Jenkner <wjenkner@inode.at>
Date: Thu, 8 Jan 2015 21:13:44 +0100
Subject: [PATCH] Handle the `neg' operator in some calc-units functions.

* lisp/calc/calc-units.el (math-units-in-expr-p)
(math-single-units-in-expr-p, math-find-compatible-unit-rec)
(math-extract-units): Handle the `neg' operator.
* test/automated/calc-tests.el (calc-tests-equal, calc-tests-simple):
New functions.
(test-calc-remove-units, test-calc-extract-units)
(test-calc-convert-units): New tests.
---
 lisp/ChangeLog               |  6 ++++++
 lisp/calc/calc-units.el      | 16 ++++++++++----
 test/ChangeLog               |  7 +++++++
 test/automated/calc-tests.el | 50 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 4985ad1..34136e4 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-08  Wolfgang Jenkner  <wjenkner@inode.at>
+
+	* calc/calc-units.el (math-units-in-expr-p)
+	(math-single-units-in-expr-p, math-find-compatible-unit-rec)
+	(math-extract-units): Handle the `neg' operator.
+
 2015-01-12  Martin Rudalics  <rudalics@gmx.at>
 
 	* progmodes/xref.el (xref-marker-stack-empty-p): Add autoload
diff --git a/lisp/calc/calc-units.el b/lisp/calc/calc-units.el
index 26a644a..0595086 100644
--- a/lisp/calc/calc-units.el
+++ b/lisp/calc/calc-units.el
@@ -904,10 +904,12 @@ If COMP or STD is non-nil, put that in the units table instead."
   (and (consp expr)
        (if (eq (car expr) 'var)
 	   (math-check-unit-name expr)
-	 (and (or sub-exprs
-		  (memq (car expr) '(* / ^)))
-	      (or (math-units-in-expr-p (nth 1 expr) sub-exprs)
-		  (math-units-in-expr-p (nth 2 expr) sub-exprs))))))
+	 (if (eq (car expr) 'neg)
+	     (math-units-in-expr-p (nth 1 expr) sub-exprs)
+	   (and (or sub-exprs
+		    (memq (car expr) '(* / ^)))
+		(or (math-units-in-expr-p (nth 1 expr) sub-exprs)
+		    (math-units-in-expr-p (nth 2 expr) sub-exprs)))))))
 
 (defun math-only-units-in-expr-p (expr)
   (and (consp expr)
@@ -924,6 +926,8 @@ If COMP or STD is non-nil, put that in the units table instead."
   (cond ((math-scalarp expr) nil)
 	((eq (car expr) 'var)
 	 (math-check-unit-name expr))
+	((eq (car expr) 'neg)
+	 (math-single-units-in-expr-p (nth 1 expr)))
 	((eq (car expr) '*)
 	 (let ((u1 (math-single-units-in-expr-p (nth 1 expr)))
 	       (u2 (math-single-units-in-expr-p (nth 2 expr))))
@@ -1079,6 +1083,8 @@ If COMP or STD is non-nil, put that in the units table instead."
 	((eq (car-safe expr) '/)
 	 (or (math-find-compatible-unit-rec (nth 1 expr) pow)
 	     (math-find-compatible-unit-rec (nth 2 expr) (- pow))))
+	((eq (car-safe expr) 'neg)
+	 (math-find-compatible-unit-rec (nth 1 expr) pow))
 	((and (eq (car-safe expr) '^)
 	      (integerp (nth 2 expr)))
 	 (math-find-compatible-unit-rec (nth 1 expr) (* pow (nth 2 expr))))
@@ -1497,6 +1503,8 @@ If COMP or STD is non-nil, put that in the units table instead."
    ((memq (car-safe expr) '(* /))
     (cons (car expr)
           (mapcar 'math-extract-units (cdr expr))))
+   ((eq (car-safe expr) 'neg)
+    (math-extract-units (nth 1 expr)))
    ((eq (car-safe expr) '^)
     (list '^ (math-extract-units (nth 1 expr)) (nth 2 expr)))
    ((math-check-unit-name expr) expr)
diff --git a/test/ChangeLog b/test/ChangeLog
index 83bb8bf..527338b 100644
--- a/test/ChangeLog
+++ b/test/ChangeLog
@@ -1,3 +1,10 @@
+2015-01-08  Wolfgang Jenkner  <wjenkner@inode.at>
+
+	* automated/calc-tests.el (calc-tests-equal, calc-tests-simple):
+	New functions.
+	(test-calc-remove-units, test-calc-extract-units)
+	(test-calc-convert-units): New tests.
+
 2015-01-08  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* automated/eieio-tests.el (eieio-test-23-inheritance-check): Don't use
diff --git a/test/automated/calc-tests.el b/test/automated/calc-tests.el
index 331e01e..d134d0a 100644
--- a/test/automated/calc-tests.el
+++ b/test/automated/calc-tests.el
@@ -27,6 +27,38 @@
 (require 'cl-lib)
 (require 'ert)
 (require 'calc)
+(require 'calc-ext)
+(require 'calc-units)
+
+;; XXX Apparently, the representation of numbers is sometimes
+;; influenced by previous results in the same calc stack.  Hence this
+;; function.
+(defun calc-tests-equal (a b)
+  "Like `equal' but allow for different representations of numbers.
+For example: (calc-tests-equal 10 '(float 1 1)) => t.
+A and B should be calc expressions."
+  (cond ((math-numberp a)
+	 (and (math-numberp b)
+	      (math-equal a b)))
+	((atom a)
+	 (equal a b))
+	((consp b)
+	 ;; Can't be dotted or circular.
+	 (and (= (length a) (length b))
+	      (equal (car a) (car b))
+	      (cl-every #'calc-tests-equal (cdr a) (cdr b))))))
+
+(defun calc-tests-simple (fun string &rest args)
+  "Push STRING on the calc stack, then call FUN and return the new top.
+The result is a calc (i.e., lisp) expression, not its string representation.
+Also pop the entire stack afterwards.
+An existing calc stack is reused, otherwise a new one is created."
+  (calc-eval string 'push)
+  (prog1
+      (ignore-errors
+	(apply fun args)
+	(calc-top-n 1))
+    (calc-pop 0)))
 
 (ert-deftest test-math-bignum ()
   ;; bug#17556
@@ -34,6 +66,24 @@
     (should (math-negp n))
     (should (cl-notany #'cl-minusp (cdr n)))))
 
+(ert-deftest test-calc-remove-units ()
+  (should (calc-tests-equal (calc-tests-simple #'calc-remove-units "-1 m") -1)))
+
+(ert-deftest test-calc-extract-units ()
+  (should (calc-tests-equal (calc-tests-simple #'calc-extract-units "-1 m")
+			    '(var m var-m)))
+  (should (calc-tests-equal (calc-tests-simple #'calc-extract-units "-1 m*cm")
+			    '(* (float 1 -2) (^ (var m var-m) 2)))))
+
+(ert-deftest test-calc-convert-units ()
+  ;; Used to ask for `(The expression is unitless when simplified) Old Units: '.
+  (should (calc-tests-equal (calc-tests-simple #'calc-convert-units "-1 m" nil "cm")
+			    '(* -100 (var cm var-cm))))
+  ;; Gave wrong result.
+  (should (calc-tests-equal (calc-tests-simple #'calc-convert-units "-1 m"
+					       (math-read-expr "1m") "cm")
+			    '(* -100 (var cm var-cm)))))
+
 (provide 'calc-tests)
 ;;; calc-tests.el ends here
 
-- 
2.2.1

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Jay Belanger]

> I propose the attached patch,

Fantastic, thanks.  I'll apply the patches to Calc as soon as I get to
my home computer.

But what did you mean when you wrote:
   Apparently, the representation of numbers is sometimes
   influenced by previous results in the same calc stack.
?

Jay

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Wolfgang Jenkner]

On Tue, Jan 13 2015, Jay Belanger wrote:

>> I propose the attached patch,
>
> Fantastic, thanks.  I'll apply the patches to Calc as soon as I get to
> my home computer.

I could do this myself, actually...

> But what did you mean when you wrote:
>    Apparently, the representation of numbers is sometimes
>    influenced by previous results in the same calc stack.

The same calculation can return an expression where some sub-expression
is a plain integer, say 100, or the corresponding float (float 1 2).
I'll provide an explicit example.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Jay Belanger]

>> Fantastic, thanks.  I'll apply the patches to Calc as soon as I get to
>> my home computer.
>
> I could do this myself, actually...

Great!

>> But what did you mean when you wrote:
>>    Apparently, the representation of numbers is sometimes
>>    influenced by previous results in the same calc stack.
>
> The same calculation can return an expression where some sub-expression
> is a plain integer, say 100, or the corresponding float (float 1 2).
> I'll provide an explicit example.

Okay, thanks.  Since 100 and (float 1 2) aren't the same, that might be
a bug of some kind.  I'm not sure that a test should indicate that they
are equal, so maybe you shouldn't put that part in yet.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Wolfgang Jenkner]

On Tue, Jan 13 2015, Jay Belanger wrote:

> Since 100 and (float 1 2) aren't the same, that might be a bug of some
> kind.

Sure, but

(math-equal 100 '(float 1 2)) => t

and (info "(calc) Predicates") states

 -- Function: math-equal x y
     Returns true if X and Y are numerically equal, either because they
     are `equal', or because their difference is `zerop'. [...]

so, IIUC, mathematically, they are as equal as it gets in calc.

On the other hand, I'm not sure if calculations with (float ...)
are exact (if possible) in calc ?
 

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Jay Belanger]

>> Since 100 and (float 1 2) aren't the same, that might be a bug of some
>> kind.
>
> Sure, but
>
> (math-equal 100 '(float 1 2)) => t
>
> and (info "(calc) Predicates") states
>
>  -- Function: math-equal x y
>      Returns true if X and Y are numerically equal, either because they
>      are `equal', or because their difference is `zerop'. [...]
>
> so, IIUC, mathematically, they are as equal as it gets in calc.

Ah, right.  But the same calculations on the same objects should produce
results which are equal, not just math-equal.  

> On the other hand, I'm not sure if calculations with (float ...)
> are exact (if possible) in calc ?

Calc will do the exact computations, since internally it uses base 10 to
represents floats.  Of course, the user might not consider (float ...)s
to be exact.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Jay Belanger]

> Ah, right.  But the same calculations on the same objects should produce
> results which are equal, not just math-equal.  

... an example will probably clarify the issue.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Wolfgang Jenkner]

On Tue, Jan 13 2015, Wolfgang Jenkner wrote:

> The same calculation can return an expression where some sub-expression
> is a plain integer, say 100, or the corresponding float (float 1 2).
> I'll provide an explicit example.

So the issue seems to be this:

In emacs -Q, do `M-x calc' then convert, say 1m, to cm, i.e.,

' 1 m <return> u c c m <return>

This gives `100 cm' on top of the stack (a plain integer scalar).

Next, start another emacs -Q instance, do `M-x calc' and then, before
anything else, `M-: (load-library "calc-units")'.

Now the same conversion exercise as before gives `100. cm' on the stack
instead (a calc float).

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Jay Belanger]

> So the issue seems to be this:
>
> In emacs -Q, do `M-x calc' then convert, say 1m, to cm, i.e.,
>
> ' 1 m <return> u c c m <return>
>
> This gives `100 cm' on top of the stack (a plain integer scalar).
>
> Next, start another emacs -Q instance, do `M-x calc' and then, before
> anything else, `M-: (load-library "calc-units")'.
>
> Now the same conversion exercise as before gives `100. cm' on the stack
> instead (a calc float).

Huh.
Thanks.
Well, now I see what you need to use math-equal.

As for the float, I'll see if I can fix that before the next release.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Wolfgang Jenkner]

On Tue, Jan 13 2015, Jay Belanger wrote:

>>> Fantastic, thanks.  I'll apply the patches to Calc as soon as I get to
>>> my home computer.
>>
>> I could do this myself, actually...
>
> Great!

I've committed this now.  I only corrected the comment before
calc-tests-equal in test/automated/calc-tests.el.

Since I'm working on making the simplification functions in calc-units
aware of the (neg ...) expressions where needed, I'd like to keep this
bug report open a bit longer so that I can send here the patch for
review.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Wolfgang Jenkner]

On Fri, Jan 16 2015, Wolfgang Jenkner wrote:

> I've committed this now.  

...to master as

  commit 2290000ed69b1157739c280f090e5b60112e83fe
  Author: Wolfgang Jenkner <wjenkner@inode.at>
  Date:   Thu Jan 15 20:02:17 2015 +0100

  Handle the `neg' operator in some calc-units functions.

However, according to

http://permalink.gmane.org/gmane.emacs.devel/181383

it falls within the category of things that should go into emacs-24
instead, so I guess I should backport it.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Jay Belanger]

> So the issue seems to be this:
>
> In emacs -Q, do `M-x calc' then convert, say 1m, to cm, i.e.,
>
> ' 1 m <return> u c c m <return>
>
> This gives `100 cm' on top of the stack (a plain integer scalar).
>
> Next, start another emacs -Q instance, do `M-x calc' and then, before
> anything else, `M-: (load-library "calc-units")'.
>
> Now the same conversion exercise as before gives `100. cm' on the stack
> instead (a calc float).

This is easy enough to fix, but loading error occurs when the libraries
are loaded out of order; that shouldn't be done.
When Calc is used properly, getting `100. m' instead of `100 m' would be a
bug.

bug#19582: 25.0.50; [PATCH] Calc: glitches with negation of units

[Lars Ingebrigtsen]

Wolfgang Jenkner <wjenkner@inode.at> writes:

> Since I'm working on making the simplification functions in calc-units
> aware of the (neg ...) expressions where needed, I'd like to keep this
> bug report open a bit longer so that I can send here the patch for
> review.

That was a year ago...  I'm assuming this was fixed by now?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no