[PATCH] org-table: Add mode flag to enable Calc units simplification mode

[PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

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

Hello,

attached there are a few patches reworking the parsing of org-table
formula mode strings and introducing a new mode flag to enable Calc's
units simplification mode as discussed in a recent thread on the mailing
list. I haven't updated the documentation. I can do it once we agree
that this feature is a good idea.

I have already submitted the patches in another thread
https://orgmode.org/list/6d8c15c2-d1b0-d913-df39-c60381cff70b@grinta.net/T/#m03a426dd8476b60019dfffecb8781a2126df690f
but it seems that woof did not pick them up, thus I am re-sending them.

Cheers,
Dan

[-- Attachment #2: 0001-org-table-Fix-table-formula-mode-string-handling.patch --]
[-- Type: text/plain, Size: 2160 bytes --]

From c7434974897d932fe3acd182f06a98a61719e208 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 11:03:14 +0200
Subject: [PATCH 1/4] org-table: Fix table formula mode string handling

* lisp/org-table.el (org-table-eval-formula): Move mode lookup table
from org-table--set-calc-mode to here.

* lisp/org-table.el (org-table--set-calc-mode): Use plist-put instead
of the buggy open coded version.
---
 lisp/org-table.el | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 112b1e171..0790dc3ca 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -721,17 +721,8 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var &optional value)
-  (if (stringp var)
-      (setq var (assoc var '(("D" calc-angle-mode deg)
-			     ("R" calc-angle-mode rad)
-			     ("F" calc-prefer-frac t)
-			     ("S" calc-symbolic-mode t)))
-	    value (nth 2 var) var (nth 1 var)))
-  (if (memq var org-tbl-calc-modes)
-      (setcar (cdr (memq var org-tbl-calc-modes)) value)
-    (cons var (cons value org-tbl-calc-modes)))
-  org-tbl-calc-modes)
+(defsubst org-table--set-calc-mode (var value)
+  (plist-put org-tbl-calc-modes var value))
 
 \f
 ;;; Predicates
@@ -2476,9 +2467,14 @@ location of point."
 		(setq keep-empty t
 		      fmt (replace-match "" t t fmt)))
 	    (while (string-match "[DRFS]" fmt)
-	      (setq org-tbl-calc-modes
-		    (org-table--set-calc-mode (match-string 0 fmt)))
-	      (setq fmt (replace-match "" t t fmt)))
+	      (let* ((c (string-to-char (match-string 0 fmt)))
+		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
+					   (?R calc-angle-mode rad)
+					   (?F calc-prefer-frac t)
+					   (?S calc-symbolic-mode t))))))
+		(setq org-tbl-calc-modes (org-table--set-calc-mode
+					  (car mode) (cadr mode))
+		      fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
-- 
2.28.0


[-- Attachment #3: 0002-org-table-Simplify-mode-string-parsing.patch --]
[-- Type: text/plain, Size: 5850 bytes --]

From fb8b62e5faabca2b6c6514e25bd306f7a5e8696f Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 15:13:40 +0200
Subject: [PATCH 2/4] org-table: Simplify mode string parsing

* org-table.el (org-table-eval-formula): Simplify mode string parsing
and reduce scope of local variables.
---
 lisp/org-table.el | 98 +++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 55 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 0790dc3ca..4baad2600 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -722,7 +722,7 @@ Field is restored even in case of abnormal exit."
 	 (set-marker ,line nil)))))
 
 (defsubst org-table--set-calc-mode (var value)
-  (plist-put org-tbl-calc-modes var value))
+  (setq org-tbl-calc-modes (plist-put org-tbl-calc-modes var value)))
 
 \f
 ;;; Predicates
@@ -2427,54 +2427,42 @@ location of point."
 	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
-	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
+	   form form0 formrpl formrg bw fmt ev orig lispp literal
 	   duration duration-output-format)
       ;; Parse the format string.  Since we have a lot of modes, this is
       ;; a lot of work.  However, I think calc still uses most of the time.
-      (if (string-match ";" formula)
-	  (let ((tmp (org-split-string formula ";")))
-	    (setq formula (car tmp)
-		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
-			      (nth 1 tmp)))
+      (if (string-match "\\(.*\\);\\(.*\\)" formula)
+	  (progn
+	    (setq fmt (concat (match-string-no-properties 2 formula)
+			      (cdr (assoc "%" org-table-local-parameters)))
+		  formula (match-string-no-properties 1 formula))
 	    (while (string-match "\\([pnfse]\\)\\(-?[0-9]+\\)" fmt)
-	      (setq c (string-to-char (match-string 1 fmt))
-		    n (string-to-number (match-string 2 fmt)))
-	      (if (= c ?p)
-		  (setq org-tbl-calc-modes
-			(org-table--set-calc-mode 'calc-internal-prec n))
-		(setq org-tbl-calc-modes
-		      (org-table--set-calc-mode
-		       'calc-float-format
-		       (list (cdr (assoc c '((?n . float) (?f . fix)
-					     (?s . sci) (?e . eng))))
-			     n))))
+	      (let ((c (string-to-char (match-string 1 fmt)))
+		    (n (string-to-number (match-string 2 fmt))))
+		(cl-case c
+		  (?p (org-table--set-calc-mode 'calc-internal-prec n))
+		  (?n (org-table--set-calc-mode 'calc-float-format (list 'float n)))
+		  (?f (org-table--set-calc-mode 'calc-float-format (list 'fix n)))
+		  (?s (org-table--set-calc-mode 'calc-float-format (list 'sci n)))
+		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
+	      ;; Remove matched flags from the mode string.
+	      (setq fmt (replace-match "" t t fmt)))
+	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	      (let ((c (string-to-char (match-string 1 fmt))))
+		(cl-case c
+		  (?t (setq duration t numbers t
+		      	    duration-output-format org-table-duration-custom-format))
+		  (?T (setq duration t numbers t duration-output-format nil))
+		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
+		  (?N (setq numbers t))
+		  (?L (setq literal t))
+		  (?E (setq keep-empty t))
+		  (?D (org-table--set-calc-mode 'calc-angle-mode 'deg))
+		  (?R (org-table--set-calc-mode 'calc-angle-mode 'rad))
+		  (?F (org-table--set-calc-mode 'calc-prefer-frac t))
+		  (?S (org-table--set-calc-mode 'calc-symbolic-mode t))))
+	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (if (string-match "[tTU]" fmt)
-		(let ((ff (match-string 0 fmt)))
-		  (setq duration t numbers t
-			duration-output-format
-			(cond ((equal ff "T") nil)
-			      ((equal ff "t") org-table-duration-custom-format)
-			      ((equal ff "U") 'hh:mm))
-			fmt (replace-match "" t t fmt))))
-	    (if (string-match "N" fmt)
-		(setq numbers t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "L" fmt)
-		(setq literal t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "E" fmt)
-		(setq keep-empty t
-		      fmt (replace-match "" t t fmt)))
-	    (while (string-match "[DRFS]" fmt)
-	      (let* ((c (string-to-char (match-string 0 fmt)))
-		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
-					   (?R calc-angle-mode rad)
-					   (?F calc-prefer-frac t)
-					   (?S calc-symbolic-mode t))))))
-		(setq org-tbl-calc-modes (org-table--set-calc-mode
-					  (car mode) (cadr mode))
-		      fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2575,17 +2563,17 @@ location of point."
 	(setq form0 form)
 	;; Insert the references to fields in same row
 	(while (string-match "\\$\\(\\([-+]\\)?[0-9]+\\)" form)
-	  (setq n (+ (string-to-number (match-string 1 form))
-		     (if (match-end 2) n0 0))
-		x (nth (1- (if (= n 0) n0 (max n 1))) fields)
-		formrpl (save-match-data
-			  (org-table-make-reference
-			   x keep-empty numbers lispp)))
-	  (when (or (not x)
-		    (save-match-data
-		      (string-match (regexp-quote formula) formrpl)))
-	    (user-error "Invalid field specifier \"%s\""
-			(match-string 0 form)))
+	  (let* ((n (+ (string-to-number (match-string 1 form))
+		       (if (match-end 2) n0 0)))
+		 (x (nth (1- (if (= n 0) n0 (max n 1))) fields)))
+	    (setq formrpl (save-match-data
+			    (org-table-make-reference
+			     x keep-empty numbers lispp)))
+	    (when (or (not x)
+		      (save-match-data
+			(string-match (regexp-quote formula) formrpl)))
+	      (user-error "Invalid field specifier \"%s\""
+			  (match-string 0 form))))
 	  (setq form (replace-match formrpl t t form)))
 
 	(if lispp
-- 
2.28.0


[-- Attachment #4: 0003-org-table-Add-mode-flag-to-enable-Calc-units-simplif.patch --]
[-- Type: text/plain, Size: 1301 bytes --]

From aad8eb548e7a7a7fde1908a9f9c66f980da10b56 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 15:22:02 +0200
Subject: [PATCH 3/4] org-table: Add mode flag to enable Calc units
 simplification mode

* org-table.el (org-table-eval-formula): Add the `u` mode flag to
enable Calc's units simplification mode.
---
 lisp/org-table.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 4baad2600..6b92656bd 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2447,11 +2447,12 @@ location of point."
 		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
 	      (let ((c (string-to-char (match-string 1 fmt))))
 		(cl-case c
 		  (?t (setq duration t numbers t
 		      	    duration-output-format org-table-duration-custom-format))
+		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
 		  (?T (setq duration t numbers t duration-output-format nil))
 		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
 		  (?N (setq numbers t))
-- 
2.28.0


[-- Attachment #5: 0004-org-table-Remove-unused-org-tbl-calc-modes-variable.patch --]
[-- Type: text/plain, Size: 4374 bytes --]

From 2d4521a032ec3e4174c97b2b2e9a08491e9870fb Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Wed, 21 Oct 2020 17:47:15 +0200
Subject: [PATCH 4/4] org-table: Remove unused org-tbl-calc-modes variable

* org-table.el (org-tbl-calc-modes): Remove the variable declaration
as the varialble is only used as a local variable in `org-table-eval-formula'.

* org-table.el (org-table--set-calc-mode): Drop convenience macro.

* org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
local variable without the org-table prefix and usr the gained screen
real estate to avoid indirection through covenience macro.
---
 lisp/org-table.el | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 6b92656bd..1651decd3 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -676,8 +676,6 @@ Will be filled automatically during use.")
     ("_" . "Names for values in row below this one.")
     ("^" . "Names for values in row above this one.")))
 
-(defvar org-tbl-calc-modes nil)
-
 (defvar org-pos nil)
 
 \f
@@ -721,9 +719,6 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var value)
-  (setq org-tbl-calc-modes (plist-put org-tbl-calc-modes var value)))
-
 \f
 ;;; Predicates
 
@@ -2424,7 +2419,7 @@ location of point."
 			equation
 		      (org-table-get-formula equation (equal arg '(4)))))
 	   (n0 (org-table-current-column))
-	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
+	   (calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
 	   form form0 formrpl formrg bw fmt ev orig lispp literal
@@ -2440,11 +2435,11 @@ location of point."
 	      (let ((c (string-to-char (match-string 1 fmt)))
 		    (n (string-to-number (match-string 2 fmt))))
 		(cl-case c
-		  (?p (org-table--set-calc-mode 'calc-internal-prec n))
-		  (?n (org-table--set-calc-mode 'calc-float-format (list 'float n)))
-		  (?f (org-table--set-calc-mode 'calc-float-format (list 'fix n)))
-		  (?s (org-table--set-calc-mode 'calc-float-format (list 'sci n)))
-		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
+		  (?p (setf (cl-getf calc-modes 'calc-internal-prec) n))
+		  (?n (setf (cl-getf calc-modes 'calc-float-format) (list 'float n)))
+		  (?f (setf (cl-getf calc-modes 'calc-float-format) (list 'fix n)))
+		  (?s (setf (cl-getf calc-modes 'calc-float-format) (list 'sci n)))
+		  (?e (setf (cl-getf calc-modes 'calc-float-format) (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
@@ -2452,16 +2447,16 @@ location of point."
 		(cl-case c
 		  (?t (setq duration t numbers t
 		      	    duration-output-format org-table-duration-custom-format))
-		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
+		  (?u (setf (cl-getf calc-modes 'calc-simplify-mode) 'units))
 		  (?T (setq duration t numbers t duration-output-format nil))
 		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
 		  (?N (setq numbers t))
 		  (?L (setq literal t))
 		  (?E (setq keep-empty t))
-		  (?D (org-table--set-calc-mode 'calc-angle-mode 'deg))
-		  (?R (org-table--set-calc-mode 'calc-angle-mode 'rad))
-		  (?F (org-table--set-calc-mode 'calc-prefer-frac t))
-		  (?S (org-table--set-calc-mode 'calc-symbolic-mode t))))
+		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
+		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
+		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (unless (string-match "\\S-" fmt)
@@ -2606,7 +2601,7 @@ location of point."
 
 	  (setq ev (if (and duration (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" form))
 		       form
-		     (calc-eval (cons form org-tbl-calc-modes)
+		     (calc-eval (cons form calc-modes)
 				(when (and (not keep-empty) numbers) 'num)))
 		ev (if duration (org-table-time-seconds-to-string
 				 (if (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" ev)
-- 
2.28.0

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

Hello,

I don't think this is what is holding up review of these patches, but, I
recently completed the paperwork for copyright assignment to the FSF.

Cheers,
Dan


On 24/10/2020 17:33, Daniele Nicolodi wrote:
> Hello,
> 
> attached there are a few patches reworking the parsing of org-table
> formula mode strings and introducing a new mode flag to enable Calc's
> units simplification mode as discussed in a recent thread on the mailing
> list. I haven't updated the documentation. I can do it once we agree
> that this feature is a good idea.
> 
> I have already submitted the patches in another thread
> https://orgmode.org/list/6d8c15c2-d1b0-d913-df39-c60381cff70b@grinta.net/T/#m03a426dd8476b60019dfffecb8781a2126df690f
> but it seems that woof did not pick them up, thus I am re-sending them.
> 
> Cheers,
> Dan
> 

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Kyle Meyer]

Daniele Nicolodi writes:

> Hello,
>
> I don't think this is what is holding up review of these patches, but, I
> recently completed the paperwork for copyright assignment to the FSF.

Thanks for this series (and thanks to Eric for the feedback in the
previous thread).  I'm sorry for the slow response.

Quickly scanning through and playing around with it, it looks nicely
done.  I'll put aside some time this weekend to give it a closer review.

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 19/11/2020 06:58, Kyle Meyer wrote:
> Daniele Nicolodi writes:
> 
>> Hello,
>>
>> I don't think this is what is holding up review of these patches, but, I
>> recently completed the paperwork for copyright assignment to the FSF.
> 
> Thanks for this series (and thanks to Eric for the feedback in the
> previous thread).  I'm sorry for the slow response.
> 
> Quickly scanning through and playing around with it, it looks nicely
> done.  I'll put aside some time this weekend to give it a closer review.

Thanks for having a look, Kyle.

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Kyle Meyer]

Daniele Nicolodi writes:

> Subject: [PATCH 1/4] org-table: Fix table formula mode string handling
>
> * lisp/org-table.el (org-table-eval-formula): Move mode lookup table
> from org-table--set-calc-mode to here.
>
> * lisp/org-table.el (org-table--set-calc-mode): Use plist-put instead
> of the buggy open coded version.

So I think the "buggy" is referring to your analysis in
<6d8c15c2-d1b0-d913-df39-c60381cff70b@grinta.net>:

  The first (if ...) does some value substitutions, however, IIUC the
  second (if ...) sets a new value for an entry in the org-tbl-calc-modes
  plist if the entry is already present and builds a new plist with the
  entry prepended if the entry is not there. However, the original plist
  is returned and not the one with the new entry prepended.

And...

> ---
>  lisp/org-table.el | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/lisp/org-table.el b/lisp/org-table.el
> index 112b1e171..0790dc3ca 100644
> --- a/lisp/org-table.el
> +++ b/lisp/org-table.el
> @@ -721,17 +721,8 @@ Field is restored even in case of abnormal exit."
>  	 (org-table-goto-column ,column)
>  	 (set-marker ,line nil)))))
>  
> -(defsubst org-table--set-calc-mode (var &optional value)
> -  (if (stringp var)
> -      (setq var (assoc var '(("D" calc-angle-mode deg)
> -			     ("R" calc-angle-mode rad)
> -			     ("F" calc-prefer-frac t)
> -			     ("S" calc-symbolic-mode t)))
> -	    value (nth 2 var) var (nth 1 var)))
> -  (if (memq var org-tbl-calc-modes)
> -      (setcar (cdr (memq var org-tbl-calc-modes)) value)
> -    (cons var (cons value org-tbl-calc-modes)))
> -  org-tbl-calc-modes)
> +(defsubst org-table--set-calc-mode (var value)
> +  (plist-put org-tbl-calc-modes var value))

...that does look to be the case.  Do you have an example that triggers
the issue?  If so, it'd be good to cover that in test-org-table.el.

However, looking ahead, org-table--set-calc-mode is dropped in the last
patch.  I'd suggest instead dropping org-table--set-calc-mode and moving
to using cl-getf as part of this first patch.  (I know that'd require a
bit of reworking since it touches changes from patches 2 and 3.)

> Subject: [PATCH 2/4] org-table: Simplify mode string parsing
>
[...]
>  ;;; Predicates
> @@ -2427,54 +2427,42 @@ location of point."
>  	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
>  	   (numbers nil)	   ; was a variable, now fixed default
>  	   (keep-empty nil)
> -	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
> +	   form form0 formrpl formrg bw fmt ev orig lispp literal
>  	   duration duration-output-format)
>        ;; Parse the format string.  Since we have a lot of modes, this is
>        ;; a lot of work.  However, I think calc still uses most of the time.
> -      (if (string-match ";" formula)
> -	  (let ((tmp (org-split-string formula ";")))
> -	    (setq formula (car tmp)
> -		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
> -			      (nth 1 tmp)))
> +      (if (string-match "\\(.*\\);\\(.*\\)" formula)
> +	  (progn
> +	    (setq fmt (concat (match-string-no-properties 2 formula)
> +			      (cdr (assoc "%" org-table-local-parameters)))
> +		  formula (match-string-no-properties 1 formula))

This patch's changes look good to me.  As a minor comment, I'd prefer if
the rewritten parts (here and for the entire series) only used one pair
per setq call, even if it's not worth updating the entire function to
use this style.

> Subject: [PATCH 3/4] org-table: Add mode flag to enable Calc units
>  simplification mode
>
> * org-table.el (org-table-eval-formula): Add the `u` mode flag to
> enable Calc's units simplification mode.

Neat.  As far as I can tell, this works nicely.

> ---
>  lisp/org-table.el | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/org-table.el b/lisp/org-table.el
> index 4baad2600..6b92656bd 100644
> --- a/lisp/org-table.el
> +++ b/lisp/org-table.el
> @@ -2447,11 +2447,12 @@ location of point."
>  		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
>  	      ;; Remove matched flags from the mode string.
>  	      (setq fmt (replace-match "" t t fmt)))
> -	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
> +	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
>  	      (let ((c (string-to-char (match-string 1 fmt))))
>  		(cl-case c
>  		  (?t (setq duration t numbers t
>  		      	    duration-output-format org-table-duration-custom-format))
> +		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
>  		  (?T (setq duration t numbers t duration-output-format nil))
>  		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
>  		  (?N (setq numbers t))

A nit-pick about ordering: I think it'd be better to not nestle "u" in
between "t" and "T" because it invites the reader to incorrectly assume
that "u" is somehow connected to "t", "T", and "U".

You already mentioned that you plan to add documentation.  It'd also be
good to add a test to test-org-table.el and a NEWS entry.

> Subject: [PATCH 4/4] org-table: Remove unused org-tbl-calc-modes variable
>
> * org-table.el (org-tbl-calc-modes): Remove the variable declaration
> as the varialble is only used as a local variable in `org-table-eval-formula'.
>
> * org-table.el (org-table--set-calc-mode): Drop convenience macro.
>
> * org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
> local variable without the org-table prefix and usr the gained screen
> real estate to avoid indirection through covenience macro.

s/usr/use/

Sounds good to me.  As I mentioned, I'd like to see this absorbed into
the first patch of the series.

Thanks again.

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

Hello Kyle,

thank you for the review. It is much appreciated as lisp (and Emacs lisp
in particular) is not the language I am most fluent in.

On 23/11/2020 04:14, Kyle Meyer wrote:
> Daniele Nicolodi writes:
> 
>> Subject: [PATCH 1/4] org-table: Fix table formula mode string handling
>>
>> * lisp/org-table.el (org-table-eval-formula): Move mode lookup table
>> from org-table--set-calc-mode to here.
>>
>> * lisp/org-table.el (org-table--set-calc-mode): Use plist-put instead
>> of the buggy open coded version.
> 
> So I think the "buggy" is referring to your analysis in
> <6d8c15c2-d1b0-d913-df39-c60381cff70b@grinta.net>:
> 
>   The first (if ...) does some value substitutions, however, IIUC the
>   second (if ...) sets a new value for an entry in the org-tbl-calc-modes
>   plist if the entry is already present and builds a new plist with the
>   entry prepended if the entry is not there. However, the original plist
>   is returned and not the one with the new entry prepended.
> 
> And...
> 
>> ---
>>  lisp/org-table.el | 24 ++++++++++--------------
>>  1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/lisp/org-table.el b/lisp/org-table.el
>> index 112b1e171..0790dc3ca 100644
>> --- a/lisp/org-table.el
>> +++ b/lisp/org-table.el
>> @@ -721,17 +721,8 @@ Field is restored even in case of abnormal exit."
>>  	 (org-table-goto-column ,column)
>>  	 (set-marker ,line nil)))))
>>  
>> -(defsubst org-table--set-calc-mode (var &optional value)
>> -  (if (stringp var)
>> -      (setq var (assoc var '(("D" calc-angle-mode deg)
>> -			     ("R" calc-angle-mode rad)
>> -			     ("F" calc-prefer-frac t)
>> -			     ("S" calc-symbolic-mode t)))
>> -	    value (nth 2 var) var (nth 1 var)))
>> -  (if (memq var org-tbl-calc-modes)
>> -      (setcar (cdr (memq var org-tbl-calc-modes)) value)
>> -    (cons var (cons value org-tbl-calc-modes)))
>> -  org-tbl-calc-modes)
>> +(defsubst org-table--set-calc-mode (var value)
>> +  (plist-put org-tbl-calc-modes var value))
> 
> ...that does look to be the case.  Do you have an example that triggers
> the issue?  If so, it'd be good to cover that in test-org-table.el.

calc-simplify-mode is not part of the default calc mode plist, thus
adding it to the plist does not work without this change.

> However, looking ahead, org-table--set-calc-mode is dropped in the last
> patch.  I'd suggest instead dropping org-table--set-calc-mode and moving
> to using cl-getf as part of this first patch.  (I know that'd require a
> bit of reworking since it touches changes from patches 2 and 3.)

I can do this.

>> Subject: [PATCH 2/4] org-table: Simplify mode string parsing
>>
> [...]
>>  ;;; Predicates
>> @@ -2427,54 +2427,42 @@ location of point."
>>  	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
>>  	   (numbers nil)	   ; was a variable, now fixed default
>>  	   (keep-empty nil)
>> -	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
>> +	   form form0 formrpl formrg bw fmt ev orig lispp literal
>>  	   duration duration-output-format)
>>        ;; Parse the format string.  Since we have a lot of modes, this is
>>        ;; a lot of work.  However, I think calc still uses most of the time.
>> -      (if (string-match ";" formula)
>> -	  (let ((tmp (org-split-string formula ";")))
>> -	    (setq formula (car tmp)
>> -		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
>> -			      (nth 1 tmp)))
>> +      (if (string-match "\\(.*\\);\\(.*\\)" formula)
>> +	  (progn
>> +	    (setq fmt (concat (match-string-no-properties 2 formula)
>> +			      (cdr (assoc "%" org-table-local-parameters)))
>> +		  formula (match-string-no-properties 1 formula))
> 
> This patch's changes look good to me.  As a minor comment, I'd prefer if
> the rewritten parts (here and for the entire series) only used one pair
> per setq call, even if it's not worth updating the entire function to
> use this style.

Funny. This is also the style I prefer, but I wrote the code to conform
to the style used in this context. I can fix this too.

>> Subject: [PATCH 3/4] org-table: Add mode flag to enable Calc units
>>  simplification mode
>>
>> * org-table.el (org-table-eval-formula): Add the `u` mode flag to
>> enable Calc's units simplification mode.
> 
> Neat.  As far as I can tell, this works nicely.
> 
>> ---
>>  lisp/org-table.el | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lisp/org-table.el b/lisp/org-table.el
>> index 4baad2600..6b92656bd 100644
>> --- a/lisp/org-table.el
>> +++ b/lisp/org-table.el
>> @@ -2447,11 +2447,12 @@ location of point."
>>  		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
>>  	      ;; Remove matched flags from the mode string.
>>  	      (setq fmt (replace-match "" t t fmt)))
>> -	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
>> +	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
>>  	      (let ((c (string-to-char (match-string 1 fmt))))
>>  		(cl-case c
>>  		  (?t (setq duration t numbers t
>>  		      	    duration-output-format org-table-duration-custom-format))
>> +		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
>>  		  (?T (setq duration t numbers t duration-output-format nil))
>>  		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
>>  		  (?N (setq numbers t))
> 
> A nit-pick about ordering: I think it'd be better to not nestle "u" in
> between "t" and "T" because it invites the reader to incorrectly assume
> that "u" is somehow connected to "t", "T", and "U".
> 
> You already mentioned that you plan to add documentation.  It'd also be
> good to add a test to test-org-table.el and a NEWS entry.

I thought alphabetical ordering was the most natural. Which other
ordering would make sense?

>> Subject: [PATCH 4/4] org-table: Remove unused org-tbl-calc-modes variable
>>
>> * org-table.el (org-tbl-calc-modes): Remove the variable declaration
>> as the varialble is only used as a local variable in `org-table-eval-formula'.
>>
>> * org-table.el (org-table--set-calc-mode): Drop convenience macro.
>>
>> * org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
>> local variable without the org-table prefix and usr the gained screen
>> real estate to avoid indirection through covenience macro.
> 
> s/usr/use/
> 
> Sounds good to me.  As I mentioned, I'd like to see this absorbed into
> the first patch of the series.

I'll send an updated series later today.

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Kyle Meyer]

Daniele Nicolodi writes:

> On 23/11/2020 04:14, Kyle Meyer wrote:
>> Daniele Nicolodi writes:
[...]
>>> diff --git a/lisp/org-table.el b/lisp/org-table.el
>>> index 4baad2600..6b92656bd 100644
>>> --- a/lisp/org-table.el
>>> +++ b/lisp/org-table.el
>>> @@ -2447,11 +2447,12 @@ location of point."
>>>  		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
>>>  	      ;; Remove matched flags from the mode string.
>>>  	      (setq fmt (replace-match "" t t fmt)))
>>> -	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
>>> +	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
>>>  	      (let ((c (string-to-char (match-string 1 fmt))))
>>>  		(cl-case c
>>>  		  (?t (setq duration t numbers t
>>>  		      	    duration-output-format org-table-duration-custom-format))
>>> +		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
>>>  		  (?T (setq duration t numbers t duration-output-format nil))
>>>  		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
>>>  		  (?N (setq numbers t))
>> 
>> A nit-pick about ordering: I think it'd be better to not nestle "u" in
>> between "t" and "T" because it invites the reader to incorrectly assume
>> that "u" is somehow connected to "t", "T", and "U".
>> 
>> You already mentioned that you plan to add documentation.  It'd also be
>> good to add a test to test-org-table.el and a NEWS entry.
>
> I thought alphabetical ordering was the most natural. Which other
> ordering would make sense?

I'd be more likely to agree that alphabetical is the most natural if the
list was already alphabetical, but instead it's grouped by "topic" (or
something :).  So I'd say just tacking u onto the end.  But I also don't
feel strongly about that, so I'm okay if you stick with the current
order.

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

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

An updated patch series is attached.

Thank you.

Cheers,
Dan

[-- Attachment #2: 0001-org-table-Remove-unused-org-tbl-calc-modes-variable-.patch --]
[-- Type: text/plain, Size: 4395 bytes --]

From d2ab4d06e19620c0347425861e4534cde8656543 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Mon, 23 Nov 2020 23:44:51 +0100
Subject: [PATCH 1/3] org-table: Remove unused org-tbl-calc-modes variable
 declaration

* org-table.el (org-tbl-calc-modes): Remove variable declaration as
the varialble is used only within `org-table-eval-formula'.

* org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
local variable without the `org-tbl-` prefix and use the gained screen
real estate to avoid indirection through covenience macro. This
requires moving the mode lookup table from `org-table--set-calc-mode`
to here.

* org-table.el (org-table--set-calc-mode): Drop convenience macro.
Note that the macro was not working as intended when the caller tried
to add a new entry in the plist as in this case the macro would create
a new plist with the added entry but return the old one.
---
 lisp/org-table.el | 42 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 112b1e171..a3b73a828 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -676,8 +676,6 @@ Will be filled automatically during use.")
     ("_" . "Names for values in row below this one.")
     ("^" . "Names for values in row above this one.")))
 
-(defvar org-tbl-calc-modes nil)
-
 (defvar org-pos nil)
 
 \f
@@ -721,18 +719,6 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var &optional value)
-  (if (stringp var)
-      (setq var (assoc var '(("D" calc-angle-mode deg)
-			     ("R" calc-angle-mode rad)
-			     ("F" calc-prefer-frac t)
-			     ("S" calc-symbolic-mode t)))
-	    value (nth 2 var) var (nth 1 var)))
-  (if (memq var org-tbl-calc-modes)
-      (setcar (cdr (memq var org-tbl-calc-modes)) value)
-    (cons var (cons value org-tbl-calc-modes)))
-  org-tbl-calc-modes)
-
 \f
 ;;; Predicates
 
@@ -2433,7 +2419,7 @@ location of point."
 			equation
 		      (org-table-get-formula equation (equal arg '(4)))))
 	   (n0 (org-table-current-column))
-	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
+	   (calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
 	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
@@ -2449,14 +2435,12 @@ location of point."
 	      (setq c (string-to-char (match-string 1 fmt))
 		    n (string-to-number (match-string 2 fmt)))
 	      (if (= c ?p)
-		  (setq org-tbl-calc-modes
-			(org-table--set-calc-mode 'calc-internal-prec n))
-		(setq org-tbl-calc-modes
-		      (org-table--set-calc-mode
-		       'calc-float-format
-		       (list (cdr (assoc c '((?n . float) (?f . fix)
-					     (?s . sci) (?e . eng))))
-			     n))))
+		  (setf (cl-getf calc-modes 'calc-internal-prec) n)
+		(setf (cl-getf calc-modes
+			       'calc-float-format)
+			       (list (cdr (assoc c '((?n . float) (?f . fix)
+						     (?s . sci) (?e . eng))))
+				     n)))
 	      (setq fmt (replace-match "" t t fmt)))
 	    (if (string-match "[tTU]" fmt)
 		(let ((ff (match-string 0 fmt)))
@@ -2476,9 +2460,13 @@ location of point."
 		(setq keep-empty t
 		      fmt (replace-match "" t t fmt)))
 	    (while (string-match "[DRFS]" fmt)
-	      (setq org-tbl-calc-modes
-		    (org-table--set-calc-mode (match-string 0 fmt)))
-	      (setq fmt (replace-match "" t t fmt)))
+	      (let* ((c (string-to-char (match-string 0 fmt)))
+		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
+					   (?R calc-angle-mode rad)
+					   (?F calc-prefer-frac t)
+					   (?S calc-symbolic-mode t))))))
+		(setf (cl-getf calc-modes (car mode)) (cadr mode))
+		(setq fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2621,7 +2609,7 @@ location of point."
 
 	  (setq ev (if (and duration (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" form))
 		       form
-		     (calc-eval (cons form org-tbl-calc-modes)
+		     (calc-eval (cons form calc-modes)
 				(when (and (not keep-empty) numbers) 'num)))
 		ev (if duration (org-table-time-seconds-to-string
 				 (if (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" ev)
-- 
2.29.2


[-- Attachment #3: 0002-org-table-Simplify-mode-string-parsing.patch --]
[-- Type: text/plain, Size: 5481 bytes --]

From c06e2964bdc8980488afbfdfeaa71535d1115a95 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 24 Nov 2020 00:19:09 +0100
Subject: [PATCH 2/3] org-table: Simplify mode string parsing

* org-table.el (org-table-eval-formula): Simplify mode string parsing
and reduce scope of local variables.
---
 lisp/org-table.el | 93 +++++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 51 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index a3b73a828..09ef15f2e 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2422,51 +2422,42 @@ location of point."
 	   (calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
-	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
+	   form form0 formrpl formrg bw fmt ev orig lispp literal
 	   duration duration-output-format)
       ;; Parse the format string.  Since we have a lot of modes, this is
       ;; a lot of work.  However, I think calc still uses most of the time.
-      (if (string-match ";" formula)
-	  (let ((tmp (org-split-string formula ";")))
-	    (setq formula (car tmp)
-		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
-			      (nth 1 tmp)))
+      (if (string-match "\\(.*\\);\\(.*\\)" formula)
+	  (progn
+	    (setq fmt (concat (match-string-no-properties 2 formula)
+			      (cdr (assoc "%" org-table-local-parameters))))
+	    (setq formula (match-string-no-properties 1 formula))
 	    (while (string-match "\\([pnfse]\\)\\(-?[0-9]+\\)" fmt)
-	      (setq c (string-to-char (match-string 1 fmt))
-		    n (string-to-number (match-string 2 fmt)))
-	      (if (= c ?p)
-		  (setf (cl-getf calc-modes 'calc-internal-prec) n)
-		(setf (cl-getf calc-modes
-			       'calc-float-format)
-			       (list (cdr (assoc c '((?n . float) (?f . fix)
-						     (?s . sci) (?e . eng))))
-				     n)))
+	      (let ((c (string-to-char (match-string 1 fmt)))
+		    (n (string-to-number (match-string 2 fmt))))
+		(cl-case c
+		  (?p (setf (cl-getf calc-modes 'calc-internal-prec) n))
+		  (?n (setf (cl-getf calc-modes 'calc-float-format) (list 'float n)))
+		  (?f (setf (cl-getf calc-modes 'calc-float-format) (list 'fix n)))
+		  (?s (setf (cl-getf calc-modes 'calc-float-format) (list 'sci n)))
+		  (?e (setf (cl-getf calc-modes 'calc-float-format) (list 'eng n)))))
+	      ;; Remove matched flags from the mode string.
+	      (setq fmt (replace-match "" t t fmt)))
+	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	      (let ((c (string-to-char (match-string 1 fmt))))
+		(cl-case c
+		  (?t (setq duration t numbers t
+		      	    duration-output-format org-table-duration-custom-format))
+		  (?T (setq duration t numbers t duration-output-format nil))
+		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
+		  (?N (setq numbers t))
+		  (?L (setq literal t))
+		  (?E (setq keep-empty t))
+		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
+		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
+		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
+	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (if (string-match "[tTU]" fmt)
-		(let ((ff (match-string 0 fmt)))
-		  (setq duration t numbers t
-			duration-output-format
-			(cond ((equal ff "T") nil)
-			      ((equal ff "t") org-table-duration-custom-format)
-			      ((equal ff "U") 'hh:mm))
-			fmt (replace-match "" t t fmt))))
-	    (if (string-match "N" fmt)
-		(setq numbers t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "L" fmt)
-		(setq literal t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "E" fmt)
-		(setq keep-empty t
-		      fmt (replace-match "" t t fmt)))
-	    (while (string-match "[DRFS]" fmt)
-	      (let* ((c (string-to-char (match-string 0 fmt)))
-		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
-					   (?R calc-angle-mode rad)
-					   (?F calc-prefer-frac t)
-					   (?S calc-symbolic-mode t))))))
-		(setf (cl-getf calc-modes (car mode)) (cadr mode))
-		(setq fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2567,17 +2558,17 @@ location of point."
 	(setq form0 form)
 	;; Insert the references to fields in same row
 	(while (string-match "\\$\\(\\([-+]\\)?[0-9]+\\)" form)
-	  (setq n (+ (string-to-number (match-string 1 form))
-		     (if (match-end 2) n0 0))
-		x (nth (1- (if (= n 0) n0 (max n 1))) fields)
-		formrpl (save-match-data
-			  (org-table-make-reference
-			   x keep-empty numbers lispp)))
-	  (when (or (not x)
-		    (save-match-data
-		      (string-match (regexp-quote formula) formrpl)))
-	    (user-error "Invalid field specifier \"%s\""
-			(match-string 0 form)))
+	  (let* ((n (+ (string-to-number (match-string 1 form))
+		       (if (match-end 2) n0 0)))
+		 (x (nth (1- (if (= n 0) n0 (max n 1))) fields)))
+	    (setq formrpl (save-match-data
+			    (org-table-make-reference
+			     x keep-empty numbers lispp)))
+	    (when (or (not x)
+		      (save-match-data
+			(string-match (regexp-quote formula) formrpl)))
+	      (user-error "Invalid field specifier \"%s\""
+			  (match-string 0 form))))
 	  (setq form (replace-match formrpl t t form)))
 
 	(if lispp
-- 
2.29.2


[-- Attachment #4: 0003-org-table-Add-mode-flag-to-enable-Calc-units-simplif.patch --]
[-- Type: text/plain, Size: 4006 bytes --]

From f29865bafd1849ed94f63911b3b1e6ae8bbb90dd Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 24 Nov 2020 00:49:16 +0100
Subject: [PATCH 3/3] org-table: Add mode flag to enable Calc units
 simplification mode

* org-table.el (org-table-eval-formula): Add the `u` mode flag to
enable Calc's units simplification mode.

* test-org-table.el (test-org-table/mode-string-u): Add Unit test for
the new mode flag.

* org-manual.org: Document new mode flag.
---
 doc/org-manual.org             |  8 ++++++++
 etc/ORG-NEWS                   |  7 ++++++-
 lisp/org-table.el              |  5 +++--
 testing/lisp/test-org-table.el | 12 ++++++++++++
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index be69996d5..70b748fc7 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -2075,6 +2075,14 @@ variable ~org-calc-default-modes~.
 
   Fraction and symbolic modes of Calc.
 
+- =u= ::
+
+  Units simplification mode of Calc. Calc is also a symbolic
+  calculator and is capable of working with values having an unit
+  (numerals followed by an unit string in Org table cells). This mode
+  instructs Calc to simplify the units in the computed expression
+  before returning the result.
+
 - =T=, =t=, =U= ::
 
   Duration computations in Calc or Lisp, [[*Durations and time values]].
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 889eb4aab..6f6db8e43 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -60,7 +60,7 @@ relative links within a project as follows:
 #+end_src
 
 ** New features
-*** =ob-python= improvements to =:return= header argument 
+*** =ob-python= improvements to =:return= header argument
 
 The =:return= header argument in =ob-python= now works for session
 blocks as well as non-session blocks.  Also, it now works with the
@@ -112,6 +112,11 @@ package, to convert pandas Dataframes into orgmode tables:
 | 2 | 3 | 6 |
 #+end_src
 
+*** New =u= table formula flag to enable Calc units simplification mode
+
+A new =u= mode flat for Calc formulas in Org tables has been added to
+enable Calc units simplification mode.
+
 ** Miscellaneous
 *** =org-goto-first-child= now works before first heading
 
diff --git a/lisp/org-table.el b/lisp/org-table.el
index 09ef15f2e..7ffe28e00 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2442,7 +2442,7 @@ location of point."
 		  (?e (setf (cl-getf calc-modes 'calc-float-format) (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	    (while (string-match "\\([tTUNLEDRFSu]\\)" fmt)
 	      (let ((c (string-to-char (match-string 1 fmt))))
 		(cl-case c
 		  (?t (setq duration t numbers t
@@ -2455,7 +2455,8 @@ location of point."
 		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
 		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
 		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
-		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))
+		  (?u (setf (cl-getf calc-modes 'calc-simplify-mode) 'units))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (unless (string-match "\\S-" fmt)
diff --git a/testing/lisp/test-org-table.el b/testing/lisp/test-org-table.el
index fb9d83f95..1c930c8d0 100644
--- a/testing/lisp/test-org-table.el
+++ b/testing/lisp/test-org-table.el
@@ -380,6 +380,18 @@ reference (with row).  Mode string N."
 "
      1 calc)))
 
+(ert-deftest test-org-table/mode-string-u ()
+  "Basic: verify that mode string u results in units
+simplification mode applied to Calc formulas."
+  (org-test-table-target-expect
+   "
+| 1.5 A/B | 2.0 B | |
+"
+   "
+| 1.5 A/B | 2.0 B | 3. A |
+"
+   1 "#+TBLFM: $3=$1*$2;u"))
+
 (ert-deftest test-org-table/lisp-return-value ()
   "Basic: Return value of Lisp formulas."
   (org-test-table-target-expect
-- 
2.29.2

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Kyle Meyer]

Daniele Nicolodi writes:

> Subject: [PATCH 1/3] org-table: Remove unused org-tbl-calc-modes variable
>  declaration

Looks good.

> Subject: [PATCH 2/3] org-table: Simplify mode string parsing
> and reduce scope of local variables.
[...]
> -      (if (string-match ";" formula)
> -	  (let ((tmp (org-split-string formula ";")))
> -	    (setq formula (car tmp)
> -		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
> -			      (nth 1 tmp)))
> +      (if (string-match "\\(.*\\);\\(.*\\)" formula)
> +	  (progn
> +	    (setq fmt (concat (match-string-no-properties 2 formula)
> +			      (cdr (assoc "%" org-table-local-parameters))))

Hmm, the concat arguments are getting swapped.  Intentional?

The rest looks good.

> Subject: [PATCH 3/3] org-table: Add mode flag to enable Calc units
>  simplification mode
>
> * org-table.el (org-table-eval-formula): Add the `u` mode flag to
> enable Calc's units simplification mode.
>
> * test-org-table.el (test-org-table/mode-string-u): Add Unit test for
> the new mode flag.
>
> * org-manual.org: Document new mode flag.
> ---
>  doc/org-manual.org             |  8 ++++++++
>  etc/ORG-NEWS                   |  7 ++++++-
>  lisp/org-table.el              |  5 +++--
>  testing/lisp/test-org-table.el | 12 ++++++++++++
>  4 files changed, 29 insertions(+), 3 deletions(-)

Thanks for the additions.

> diff --git a/doc/org-manual.org b/doc/org-manual.org
> index be69996d5..70b748fc7 100644
> --- a/doc/org-manual.org
> +++ b/doc/org-manual.org
> @@ -2075,6 +2075,14 @@ variable ~org-calc-default-modes~.
>  
>    Fraction and symbolic modes of Calc.
>  
> +- =u= ::
> +
> +  Units simplification mode of Calc. Calc is also a symbolic

convention nit: two spaces after a period

> +  calculator and is capable of working with values having an unit
> +  (numerals followed by an unit string in Org table cells). This mode
> +  instructs Calc to simplify the units in the computed expression
> +  before returning the result.
> +
>  - =T=, =t=, =U= ::
>  
>    Duration computations in Calc or Lisp, [[*Durations and time values]].
> diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
> index 889eb4aab..6f6db8e43 100644
> --- a/etc/ORG-NEWS
> +++ b/etc/ORG-NEWS
> @@ -60,7 +60,7 @@ relative links within a project as follows:
>  #+end_src
>  
>  ** New features
> -*** =ob-python= improvements to =:return= header argument 
> +*** =ob-python= improvements to =:return= header argument

unrelated space change

>  The =:return= header argument in =ob-python= now works for session
>  blocks as well as non-session blocks.  Also, it now works with the
> @@ -112,6 +112,11 @@ package, to convert pandas Dataframes into orgmode tables:
>  | 2 | 3 | 6 |
>  #+end_src
>  
> +*** New =u= table formula flag to enable Calc units simplification mode
> +
> +A new =u= mode flat for Calc formulas in Org tables has been added to
> +enable Calc units simplification mode.

s/flat/flag/

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

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

Thank you for the review, Kyle.

Another updated patch set is attached.

Cheers,
Dan


On 24/11/2020 06:35, Kyle Meyer wrote:
> Daniele Nicolodi writes:
> 
>> Subject: [PATCH 1/3] org-table: Remove unused org-tbl-calc-modes variable
>>  declaration
> 
> Looks good.
> 
>> Subject: [PATCH 2/3] org-table: Simplify mode string parsing
>> and reduce scope of local variables.
> [...]
>> -      (if (string-match ";" formula)
>> -	  (let ((tmp (org-split-string formula ";")))
>> -	    (setq formula (car tmp)
>> -		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
>> -			      (nth 1 tmp)))
>> +      (if (string-match "\\(.*\\);\\(.*\\)" formula)
>> +	  (progn
>> +	    (setq fmt (concat (match-string-no-properties 2 formula)
>> +			      (cdr (assoc "%" org-table-local-parameters))))
> 
> Hmm, the concat arguments are getting swapped.  Intentional?
> 
> The rest looks good.
> 
>> Subject: [PATCH 3/3] org-table: Add mode flag to enable Calc units
>>  simplification mode
>>
>> * org-table.el (org-table-eval-formula): Add the `u` mode flag to
>> enable Calc's units simplification mode.
>>
>> * test-org-table.el (test-org-table/mode-string-u): Add Unit test for
>> the new mode flag.
>>
>> * org-manual.org: Document new mode flag.
>> ---
>>  doc/org-manual.org             |  8 ++++++++
>>  etc/ORG-NEWS                   |  7 ++++++-
>>  lisp/org-table.el              |  5 +++--
>>  testing/lisp/test-org-table.el | 12 ++++++++++++
>>  4 files changed, 29 insertions(+), 3 deletions(-)
> 
> Thanks for the additions.
> 
>> diff --git a/doc/org-manual.org b/doc/org-manual.org
>> index be69996d5..70b748fc7 100644
>> --- a/doc/org-manual.org
>> +++ b/doc/org-manual.org
>> @@ -2075,6 +2075,14 @@ variable ~org-calc-default-modes~.
>>  
>>    Fraction and symbolic modes of Calc.
>>  
>> +- =u= ::
>> +
>> +  Units simplification mode of Calc. Calc is also a symbolic
> 
> convention nit: two spaces after a period
> 
>> +  calculator and is capable of working with values having an unit
>> +  (numerals followed by an unit string in Org table cells). This mode
>> +  instructs Calc to simplify the units in the computed expression
>> +  before returning the result.
>> +
>>  - =T=, =t=, =U= ::
>>  
>>    Duration computations in Calc or Lisp, [[*Durations and time values]].
>> diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
>> index 889eb4aab..6f6db8e43 100644
>> --- a/etc/ORG-NEWS
>> +++ b/etc/ORG-NEWS
>> @@ -60,7 +60,7 @@ relative links within a project as follows:
>>  #+end_src
>>  
>>  ** New features
>> -*** =ob-python= improvements to =:return= header argument 
>> +*** =ob-python= improvements to =:return= header argument
> 
> unrelated space change
> 
>>  The =:return= header argument in =ob-python= now works for session
>>  blocks as well as non-session blocks.  Also, it now works with the
>> @@ -112,6 +112,11 @@ package, to convert pandas Dataframes into orgmode tables:
>>  | 2 | 3 | 6 |
>>  #+end_src
>>  
>> +*** New =u= table formula flag to enable Calc units simplification mode
>> +
>> +A new =u= mode flat for Calc formulas in Org tables has been added to
>> +enable Calc units simplification mode.
> 
> s/flat/flag/
> 


[-- Attachment #2: 0001-org-table-Remove-unused-org-tbl-calc-modes-variable-.patch --]
[-- Type: text/plain, Size: 4395 bytes --]

From d2ab4d06e19620c0347425861e4534cde8656543 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Mon, 23 Nov 2020 23:44:51 +0100
Subject: [PATCH 1/3] org-table: Remove unused org-tbl-calc-modes variable
 declaration

* org-table.el (org-tbl-calc-modes): Remove variable declaration as
the varialble is used only within `org-table-eval-formula'.

* org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
local variable without the `org-tbl-` prefix and use the gained screen
real estate to avoid indirection through covenience macro. This
requires moving the mode lookup table from `org-table--set-calc-mode`
to here.

* org-table.el (org-table--set-calc-mode): Drop convenience macro.
Note that the macro was not working as intended when the caller tried
to add a new entry in the plist as in this case the macro would create
a new plist with the added entry but return the old one.
---
 lisp/org-table.el | 42 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 112b1e171..a3b73a828 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -676,8 +676,6 @@ Will be filled automatically during use.")
     ("_" . "Names for values in row below this one.")
     ("^" . "Names for values in row above this one.")))
 
-(defvar org-tbl-calc-modes nil)
-
 (defvar org-pos nil)
 
 \f
@@ -721,18 +719,6 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var &optional value)
-  (if (stringp var)
-      (setq var (assoc var '(("D" calc-angle-mode deg)
-			     ("R" calc-angle-mode rad)
-			     ("F" calc-prefer-frac t)
-			     ("S" calc-symbolic-mode t)))
-	    value (nth 2 var) var (nth 1 var)))
-  (if (memq var org-tbl-calc-modes)
-      (setcar (cdr (memq var org-tbl-calc-modes)) value)
-    (cons var (cons value org-tbl-calc-modes)))
-  org-tbl-calc-modes)
-
 \f
 ;;; Predicates
 
@@ -2433,7 +2419,7 @@ location of point."
 			equation
 		      (org-table-get-formula equation (equal arg '(4)))))
 	   (n0 (org-table-current-column))
-	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
+	   (calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
 	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
@@ -2449,14 +2435,12 @@ location of point."
 	      (setq c (string-to-char (match-string 1 fmt))
 		    n (string-to-number (match-string 2 fmt)))
 	      (if (= c ?p)
-		  (setq org-tbl-calc-modes
-			(org-table--set-calc-mode 'calc-internal-prec n))
-		(setq org-tbl-calc-modes
-		      (org-table--set-calc-mode
-		       'calc-float-format
-		       (list (cdr (assoc c '((?n . float) (?f . fix)
-					     (?s . sci) (?e . eng))))
-			     n))))
+		  (setf (cl-getf calc-modes 'calc-internal-prec) n)
+		(setf (cl-getf calc-modes
+			       'calc-float-format)
+			       (list (cdr (assoc c '((?n . float) (?f . fix)
+						     (?s . sci) (?e . eng))))
+				     n)))
 	      (setq fmt (replace-match "" t t fmt)))
 	    (if (string-match "[tTU]" fmt)
 		(let ((ff (match-string 0 fmt)))
@@ -2476,9 +2460,13 @@ location of point."
 		(setq keep-empty t
 		      fmt (replace-match "" t t fmt)))
 	    (while (string-match "[DRFS]" fmt)
-	      (setq org-tbl-calc-modes
-		    (org-table--set-calc-mode (match-string 0 fmt)))
-	      (setq fmt (replace-match "" t t fmt)))
+	      (let* ((c (string-to-char (match-string 0 fmt)))
+		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
+					   (?R calc-angle-mode rad)
+					   (?F calc-prefer-frac t)
+					   (?S calc-symbolic-mode t))))))
+		(setf (cl-getf calc-modes (car mode)) (cadr mode))
+		(setq fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2621,7 +2609,7 @@ location of point."
 
 	  (setq ev (if (and duration (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" form))
 		       form
-		     (calc-eval (cons form org-tbl-calc-modes)
+		     (calc-eval (cons form calc-modes)
 				(when (and (not keep-empty) numbers) 'num)))
 		ev (if duration (org-table-time-seconds-to-string
 				 (if (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" ev)
-- 
2.29.2


[-- Attachment #3: 0002-org-table-Simplify-mode-string-parsing.patch --]
[-- Type: text/plain, Size: 5481 bytes --]

From 1c2f246e438483826906e62e17d4312724d1190d Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 24 Nov 2020 00:19:09 +0100
Subject: [PATCH 2/3] org-table: Simplify mode string parsing

* org-table.el (org-table-eval-formula): Simplify mode string parsing
and reduce scope of local variables.
---
 lisp/org-table.el | 93 +++++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 51 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index a3b73a828..cf1bfa31c 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2422,51 +2422,42 @@ location of point."
 	   (calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
-	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
+	   form form0 formrpl formrg bw fmt ev orig lispp literal
 	   duration duration-output-format)
       ;; Parse the format string.  Since we have a lot of modes, this is
       ;; a lot of work.  However, I think calc still uses most of the time.
-      (if (string-match ";" formula)
-	  (let ((tmp (org-split-string formula ";")))
-	    (setq formula (car tmp)
-		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
-			      (nth 1 tmp)))
+      (if (string-match "\\(.*\\);\\(.*\\)" formula)
+	  (progn
+	    (setq fmt (concat (cdr (assoc "%" org-table-local-parameters))
+			      (match-string-no-properties 2 formula)))
+	    (setq formula (match-string-no-properties 1 formula))
 	    (while (string-match "\\([pnfse]\\)\\(-?[0-9]+\\)" fmt)
-	      (setq c (string-to-char (match-string 1 fmt))
-		    n (string-to-number (match-string 2 fmt)))
-	      (if (= c ?p)
-		  (setf (cl-getf calc-modes 'calc-internal-prec) n)
-		(setf (cl-getf calc-modes
-			       'calc-float-format)
-			       (list (cdr (assoc c '((?n . float) (?f . fix)
-						     (?s . sci) (?e . eng))))
-				     n)))
+	      (let ((c (string-to-char (match-string 1 fmt)))
+		    (n (string-to-number (match-string 2 fmt))))
+		(cl-case c
+		  (?p (setf (cl-getf calc-modes 'calc-internal-prec) n))
+		  (?n (setf (cl-getf calc-modes 'calc-float-format) (list 'float n)))
+		  (?f (setf (cl-getf calc-modes 'calc-float-format) (list 'fix n)))
+		  (?s (setf (cl-getf calc-modes 'calc-float-format) (list 'sci n)))
+		  (?e (setf (cl-getf calc-modes 'calc-float-format) (list 'eng n)))))
+	      ;; Remove matched flags from the mode string.
+	      (setq fmt (replace-match "" t t fmt)))
+	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	      (let ((c (string-to-char (match-string 1 fmt))))
+		(cl-case c
+		  (?t (setq duration t numbers t
+		      	    duration-output-format org-table-duration-custom-format))
+		  (?T (setq duration t numbers t duration-output-format nil))
+		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
+		  (?N (setq numbers t))
+		  (?L (setq literal t))
+		  (?E (setq keep-empty t))
+		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
+		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
+		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
+	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (if (string-match "[tTU]" fmt)
-		(let ((ff (match-string 0 fmt)))
-		  (setq duration t numbers t
-			duration-output-format
-			(cond ((equal ff "T") nil)
-			      ((equal ff "t") org-table-duration-custom-format)
-			      ((equal ff "U") 'hh:mm))
-			fmt (replace-match "" t t fmt))))
-	    (if (string-match "N" fmt)
-		(setq numbers t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "L" fmt)
-		(setq literal t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "E" fmt)
-		(setq keep-empty t
-		      fmt (replace-match "" t t fmt)))
-	    (while (string-match "[DRFS]" fmt)
-	      (let* ((c (string-to-char (match-string 0 fmt)))
-		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
-					   (?R calc-angle-mode rad)
-					   (?F calc-prefer-frac t)
-					   (?S calc-symbolic-mode t))))))
-		(setf (cl-getf calc-modes (car mode)) (cadr mode))
-		(setq fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2567,17 +2558,17 @@ location of point."
 	(setq form0 form)
 	;; Insert the references to fields in same row
 	(while (string-match "\\$\\(\\([-+]\\)?[0-9]+\\)" form)
-	  (setq n (+ (string-to-number (match-string 1 form))
-		     (if (match-end 2) n0 0))
-		x (nth (1- (if (= n 0) n0 (max n 1))) fields)
-		formrpl (save-match-data
-			  (org-table-make-reference
-			   x keep-empty numbers lispp)))
-	  (when (or (not x)
-		    (save-match-data
-		      (string-match (regexp-quote formula) formrpl)))
-	    (user-error "Invalid field specifier \"%s\""
-			(match-string 0 form)))
+	  (let* ((n (+ (string-to-number (match-string 1 form))
+		       (if (match-end 2) n0 0)))
+		 (x (nth (1- (if (= n 0) n0 (max n 1))) fields)))
+	    (setq formrpl (save-match-data
+			    (org-table-make-reference
+			     x keep-empty numbers lispp)))
+	    (when (or (not x)
+		      (save-match-data
+			(string-match (regexp-quote formula) formrpl)))
+	      (user-error "Invalid field specifier \"%s\""
+			  (match-string 0 form))))
 	  (setq form (replace-match formrpl t t form)))
 
 	(if lispp
-- 
2.29.2


[-- Attachment #4: 0003-org-table-Add-mode-flag-to-enable-Calc-units-simplif.patch --]
[-- Type: text/plain, Size: 3673 bytes --]

From 2842d6f0a10ee83d8d500cd3e4c3827a32418eef Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 24 Nov 2020 00:49:16 +0100
Subject: [PATCH 3/3] org-table: Add mode flag to enable Calc units
 simplification mode

* org-table.el (org-table-eval-formula): Add the `u` mode flag to
enable Calc's units simplification mode.

* test-org-table.el (test-org-table/mode-string-u): Add Unit test for
the new mode flag.

* org-manual.org: Document new mode flag.
---
 doc/org-manual.org             |  8 ++++++++
 etc/ORG-NEWS                   |  5 +++++
 lisp/org-table.el              |  5 +++--
 testing/lisp/test-org-table.el | 12 ++++++++++++
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index be69996d5..d9de5e633 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -2075,6 +2075,14 @@ variable ~org-calc-default-modes~.
 
   Fraction and symbolic modes of Calc.
 
+- =u= ::
+
+  Units simplification mode of Calc.  Calc is also a symbolic
+  calculator and is capable of working with values having an unit,
+  represented with numerals followed by an unit string in Org table
+  cells.  This mode instructs Calc to simplify the units in the
+  computed expression before returning the result.
+
 - =T=, =t=, =U= ::
 
   Duration computations in Calc or Lisp, [[*Durations and time values]].
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 889eb4aab..0d08a939c 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -112,6 +112,11 @@ package, to convert pandas Dataframes into orgmode tables:
 | 2 | 3 | 6 |
 #+end_src
 
+*** New =u= table formula flag to enable Calc units simplification mode
+
+A new =u= mode flag for Calc formulas in Org tables has been added to
+enable Calc units simplification mode.
+
 ** Miscellaneous
 *** =org-goto-first-child= now works before first heading
 
diff --git a/lisp/org-table.el b/lisp/org-table.el
index cf1bfa31c..c138190ef 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2442,7 +2442,7 @@ location of point."
 		  (?e (setf (cl-getf calc-modes 'calc-float-format) (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	    (while (string-match "\\([tTUNLEDRFSu]\\)" fmt)
 	      (let ((c (string-to-char (match-string 1 fmt))))
 		(cl-case c
 		  (?t (setq duration t numbers t
@@ -2455,7 +2455,8 @@ location of point."
 		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
 		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
 		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
-		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))
+		  (?u (setf (cl-getf calc-modes 'calc-simplify-mode) 'units))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (unless (string-match "\\S-" fmt)
diff --git a/testing/lisp/test-org-table.el b/testing/lisp/test-org-table.el
index fb9d83f95..1c930c8d0 100644
--- a/testing/lisp/test-org-table.el
+++ b/testing/lisp/test-org-table.el
@@ -380,6 +380,18 @@ reference (with row).  Mode string N."
 "
      1 calc)))
 
+(ert-deftest test-org-table/mode-string-u ()
+  "Basic: verify that mode string u results in units
+simplification mode applied to Calc formulas."
+  (org-test-table-target-expect
+   "
+| 1.5 A/B | 2.0 B | |
+"
+   "
+| 1.5 A/B | 2.0 B | 3. A |
+"
+   1 "#+TBLFM: $3=$1*$2;u"))
+
 (ert-deftest test-org-table/lisp-return-value ()
   "Basic: Return value of Lisp formulas."
   (org-test-table-target-expect
-- 
2.29.2

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Kyle Meyer]

Daniele Nicolodi writes:

> Thank you for the review, Kyle.
>
> Another updated patch set is attached.

Thank you for the update.

Applied, tweaking the manual entry to use "a unit" rather than "an
unit".

1:  bd7e16ca2 = 1:  bd7e16ca2 org-table: Remove unused org-tbl-calc-modes variable declaration
2:  abd994943 = 2:  abd994943 org-table: Simplify mode string parsing
3:  cb77e7a46 ! 3:  15469774d org-table: Add mode flag to enable Calc units simplification mode
    @@ doc/org-manual.org: *** Formula syntax for Calc
     +- =u= ::
     +
     +  Units simplification mode of Calc.  Calc is also a symbolic
    -+  calculator and is capable of working with values having an unit,
    -+  represented with numerals followed by an unit string in Org table
    ++  calculator and is capable of working with values having a unit,
    ++  represented with numerals followed by a unit string in Org table
     +  cells.  This mode instructs Calc to simplify the units in the
     +  computed expression before returning the result.
     +

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Christian Moe]

Great! Thanks for following through on this, Dan!

Yours,
Christian

Kyle Meyer writes:

> Daniele Nicolodi writes:
>
>> Thank you for the review, Kyle.
>>
>> Another updated patch set is attached.
>
> Thank you for the update.
>
> Applied, tweaking the manual entry to use "a unit" rather than "an
> unit".
>
> 1:  bd7e16ca2 = 1:  bd7e16ca2 org-table: Remove unused org-tbl-calc-modes variable declaration
> 2:  abd994943 = 2:  abd994943 org-table: Simplify mode string parsing
> 3:  cb77e7a46 ! 3:  15469774d org-table: Add mode flag to enable Calc units simplification mode
>     @@ doc/org-manual.org: *** Formula syntax for Calc
>      +- =u= ::
>      +
>      +  Units simplification mode of Calc.  Calc is also a symbolic
>     -+  calculator and is capable of working with values having an unit,
>     -+  represented with numerals followed by an unit string in Org table
>     ++  calculator and is capable of working with values having a unit,
>     ++  represented with numerals followed by a unit string in Org table
>      +  cells.  This mode instructs Calc to simplify the units in the
>      +  computed expression before returning the result.
>      +

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 23/11/2020 04:14, Kyle Meyer wrote:
> You already mentioned that you plan to add documentation.  It'd also be
> good to add a test to test-org-table.el and a NEWS entry.

By the way, have you seen my other patch with some documentation updates
for Org tables?

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Kyle Meyer]

Daniele Nicolodi writes:

> On 23/11/2020 04:14, Kyle Meyer wrote:
>> You already mentioned that you plan to add documentation.  It'd also be
>> good to add a test to test-org-table.el and a NEWS entry.
>
> By the way, have you seen my other patch with some documentation updates
> for Org tables?

Aside from recalling seeing some discussion about word choice, only in
the sense that it exists in a queue of patches that I will try to get to
if nobody else does.  Are you bringing it up because it'd be easier to
revisit this series after that patch is applied?

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 23/11/2020 23:25, Kyle Meyer wrote:
> Daniele Nicolodi writes:
> 
>> On 23/11/2020 04:14, Kyle Meyer wrote:
>>> You already mentioned that you plan to add documentation.  It'd also be
>>> good to add a test to test-org-table.el and a NEWS entry.
>>
>> By the way, have you seen my other patch with some documentation updates
>> for Org tables?
> 
> Aside from recalling seeing some discussion about word choice, only in
> the sense that it exists in a queue of patches that I will try to get to
> if nobody else does.

I am not planning to revisit the word choice as the current choice
maintain consistency within the manual and revisit all the manual for
substituting "interpolate" with something else consistently is outside
the scope of the proposed patch.

>  Are you bringing it up because it'd be easier to
> revisit this series after that patch is applied?

No, they both touch parts of the manual/code that deal with org-table
formula mode flags, but in different context. On the other hand, they
are related enough that it may be worth to review them in close
succession to amortize cognitive context switch costs :-)

Cheers,
Dan

Bug in org-table--set-calc-mode?

[Daniele Nicolodi]

Hello,

I am hacking org-table-eval-formula (see thread about monetary values in
org-tables) which uses this inline function:

(defsubst org-table--set-calc-mode (var &optional value)
  (if (stringp var)
      (setq var (assoc var '(("D" calc-angle-mode deg)
			     ("R" calc-angle-mode rad)
			     ("F" calc-prefer-frac t)
			     ("S" calc-symbolic-mode t)))
	    value (nth 2 var) var (nth 1 var)))
  (if (memq var org-tbl-calc-modes)
      (setcar (cdr (memq var org-tbl-calc-modes)) value)
    (cons var (cons value org-tbl-calc-modes)))
  org-tbl-calc-modes)

which I am not able to understand or which is not correct.

The first (if ...) does some value substitutions, however, IIUC the
second (if ...) sets a new value for an entry in the org-tbl-calc-modes
plist if the entry is already present and builds a new plist with the
entry prepended if the entry is not there. However, the original plist
is returned and not the one with the new entry prepended.

It does not seem to be the intended behavior.

Shouldn't this be simply:

(defsubst org-table--set-calc-mode (var &optional value)
  (if (stringp var)
      (setq var (assoc var '(("D" calc-angle-mode deg)
			     ("R" calc-angle-mode rad)
			     ("F" calc-prefer-frac t)
			     ("S" calc-symbolic-mode t)))
	    value (nth 2 var) var (nth 1 var)))
  (plist-put org-tbl-calc-modes var value))

or, better, the code refactored to do not use this function?

Cheers,
Dan

[PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

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

Hello,

attached there are a few patches reworking the code, fixing the bug, and
introducing a new mode flag to enable Calc's units simplification mode
as discussed in a recent thread on the mailing list.  I haven't updated
the documentation.  I can do it once we agree that this feature is a
good idea.

Cheers,
Dan


On 19/10/2020 17:38, Daniele Nicolodi wrote:
> Hello,
> 
> I am hacking org-table-eval-formula (see thread about monetary values in
> org-tables) which uses this inline function:
> 
> (defsubst org-table--set-calc-mode (var &optional value)
>   (if (stringp var)
>       (setq var (assoc var '(("D" calc-angle-mode deg)
> 			     ("R" calc-angle-mode rad)
> 			     ("F" calc-prefer-frac t)
> 			     ("S" calc-symbolic-mode t)))
> 	    value (nth 2 var) var (nth 1 var)))
>   (if (memq var org-tbl-calc-modes)
>       (setcar (cdr (memq var org-tbl-calc-modes)) value)
>     (cons var (cons value org-tbl-calc-modes)))
>   org-tbl-calc-modes)
> 
> which I am not able to understand or which is not correct.
> 
> The first (if ...) does some value substitutions, however, IIUC the
> second (if ...) sets a new value for an entry in the org-tbl-calc-modes
> plist if the entry is already present and builds a new plist with the
> entry prepended if the entry is not there. However, the original plist
> is returned and not the one with the new entry prepended.
> 
> It does not seem to be the intended behavior.
> 
> Shouldn't this be simply:
> 
> (defsubst org-table--set-calc-mode (var &optional value)
>   (if (stringp var)
>       (setq var (assoc var '(("D" calc-angle-mode deg)
> 			     ("R" calc-angle-mode rad)
> 			     ("F" calc-prefer-frac t)
> 			     ("S" calc-symbolic-mode t)))
> 	    value (nth 2 var) var (nth 1 var)))
>   (plist-put org-tbl-calc-modes var value))
> 
> or, better, the code refactored to do not use this function?
> 
> Cheers,
> Dan
> 


[-- Attachment #2: 0001-org-table-Fix-table-formula-mode-string-handling.patch --]
[-- Type: text/plain, Size: 2160 bytes --]

From c7434974897d932fe3acd182f06a98a61719e208 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 11:03:14 +0200
Subject: [PATCH 1/3] org-table: Fix table formula mode string handling

* lisp/org-table.el (org-table-eval-formula): Move mode lookup table
from org-table--set-calc-mode to here.

* lisp/org-table.el (org-table--set-calc-mode): Use plist-put instead
of the buggy open coded version.
---
 lisp/org-table.el | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 112b1e171..0790dc3ca 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -721,17 +721,8 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var &optional value)
-  (if (stringp var)
-      (setq var (assoc var '(("D" calc-angle-mode deg)
-			     ("R" calc-angle-mode rad)
-			     ("F" calc-prefer-frac t)
-			     ("S" calc-symbolic-mode t)))
-	    value (nth 2 var) var (nth 1 var)))
-  (if (memq var org-tbl-calc-modes)
-      (setcar (cdr (memq var org-tbl-calc-modes)) value)
-    (cons var (cons value org-tbl-calc-modes)))
-  org-tbl-calc-modes)
+(defsubst org-table--set-calc-mode (var value)
+  (plist-put org-tbl-calc-modes var value))
 
 \f
 ;;; Predicates
@@ -2476,9 +2467,14 @@ location of point."
 		(setq keep-empty t
 		      fmt (replace-match "" t t fmt)))
 	    (while (string-match "[DRFS]" fmt)
-	      (setq org-tbl-calc-modes
-		    (org-table--set-calc-mode (match-string 0 fmt)))
-	      (setq fmt (replace-match "" t t fmt)))
+	      (let* ((c (string-to-char (match-string 0 fmt)))
+		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
+					   (?R calc-angle-mode rad)
+					   (?F calc-prefer-frac t)
+					   (?S calc-symbolic-mode t))))))
+		(setq org-tbl-calc-modes (org-table--set-calc-mode
+					  (car mode) (cadr mode))
+		      fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
-- 
2.28.0


[-- Attachment #3: 0003-org-table-Add-mode-flag-to-enable-Calc-units-simplif.patch --]
[-- Type: text/plain, Size: 1301 bytes --]

From aad8eb548e7a7a7fde1908a9f9c66f980da10b56 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 15:22:02 +0200
Subject: [PATCH 3/3] org-table: Add mode flag to enable Calc units
 simplification mode

* org-table.el (org-table-eval-formula): Add the `u` mode flag to
enable Calc's units simplification mode.
---
 lisp/org-table.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 4baad2600..6b92656bd 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2447,11 +2447,12 @@ location of point."
 		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
 	      (let ((c (string-to-char (match-string 1 fmt))))
 		(cl-case c
 		  (?t (setq duration t numbers t
 		      	    duration-output-format org-table-duration-custom-format))
+		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
 		  (?T (setq duration t numbers t duration-output-format nil))
 		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
 		  (?N (setq numbers t))
-- 
2.28.0


[-- Attachment #4: 0002-org-table-Simplify-mode-string-parsing.patch --]
[-- Type: text/plain, Size: 5850 bytes --]

From fb8b62e5faabca2b6c6514e25bd306f7a5e8696f Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 20 Oct 2020 15:13:40 +0200
Subject: [PATCH 2/3] org-table: Simplify mode string parsing

* org-table.el (org-table-eval-formula): Simplify mode string parsing
and reduce scope of local variables.
---
 lisp/org-table.el | 98 +++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 55 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 0790dc3ca..4baad2600 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -722,7 +722,7 @@ Field is restored even in case of abnormal exit."
 	 (set-marker ,line nil)))))
 
 (defsubst org-table--set-calc-mode (var value)
-  (plist-put org-tbl-calc-modes var value))
+  (setq org-tbl-calc-modes (plist-put org-tbl-calc-modes var value)))
 
 \f
 ;;; Predicates
@@ -2427,54 +2427,42 @@ location of point."
 	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
-	   n form form0 formrpl formrg bw fmt x ev orig c lispp literal
+	   form form0 formrpl formrg bw fmt ev orig lispp literal
 	   duration duration-output-format)
       ;; Parse the format string.  Since we have a lot of modes, this is
       ;; a lot of work.  However, I think calc still uses most of the time.
-      (if (string-match ";" formula)
-	  (let ((tmp (org-split-string formula ";")))
-	    (setq formula (car tmp)
-		  fmt (concat (cdr (assoc "%" org-table-local-parameters))
-			      (nth 1 tmp)))
+      (if (string-match "\\(.*\\);\\(.*\\)" formula)
+	  (progn
+	    (setq fmt (concat (match-string-no-properties 2 formula)
+			      (cdr (assoc "%" org-table-local-parameters)))
+		  formula (match-string-no-properties 1 formula))
 	    (while (string-match "\\([pnfse]\\)\\(-?[0-9]+\\)" fmt)
-	      (setq c (string-to-char (match-string 1 fmt))
-		    n (string-to-number (match-string 2 fmt)))
-	      (if (= c ?p)
-		  (setq org-tbl-calc-modes
-			(org-table--set-calc-mode 'calc-internal-prec n))
-		(setq org-tbl-calc-modes
-		      (org-table--set-calc-mode
-		       'calc-float-format
-		       (list (cdr (assoc c '((?n . float) (?f . fix)
-					     (?s . sci) (?e . eng))))
-			     n))))
+	      (let ((c (string-to-char (match-string 1 fmt)))
+		    (n (string-to-number (match-string 2 fmt))))
+		(cl-case c
+		  (?p (org-table--set-calc-mode 'calc-internal-prec n))
+		  (?n (org-table--set-calc-mode 'calc-float-format (list 'float n)))
+		  (?f (org-table--set-calc-mode 'calc-float-format (list 'fix n)))
+		  (?s (org-table--set-calc-mode 'calc-float-format (list 'sci n)))
+		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
+	      ;; Remove matched flags from the mode string.
+	      (setq fmt (replace-match "" t t fmt)))
+	    (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
+	      (let ((c (string-to-char (match-string 1 fmt))))
+		(cl-case c
+		  (?t (setq duration t numbers t
+		      	    duration-output-format org-table-duration-custom-format))
+		  (?T (setq duration t numbers t duration-output-format nil))
+		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
+		  (?N (setq numbers t))
+		  (?L (setq literal t))
+		  (?E (setq keep-empty t))
+		  (?D (org-table--set-calc-mode 'calc-angle-mode 'deg))
+		  (?R (org-table--set-calc-mode 'calc-angle-mode 'rad))
+		  (?F (org-table--set-calc-mode 'calc-prefer-frac t))
+		  (?S (org-table--set-calc-mode 'calc-symbolic-mode t))))
+	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (if (string-match "[tTU]" fmt)
-		(let ((ff (match-string 0 fmt)))
-		  (setq duration t numbers t
-			duration-output-format
-			(cond ((equal ff "T") nil)
-			      ((equal ff "t") org-table-duration-custom-format)
-			      ((equal ff "U") 'hh:mm))
-			fmt (replace-match "" t t fmt))))
-	    (if (string-match "N" fmt)
-		(setq numbers t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "L" fmt)
-		(setq literal t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "E" fmt)
-		(setq keep-empty t
-		      fmt (replace-match "" t t fmt)))
-	    (while (string-match "[DRFS]" fmt)
-	      (let* ((c (string-to-char (match-string 0 fmt)))
-		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
-					   (?R calc-angle-mode rad)
-					   (?F calc-prefer-frac t)
-					   (?S calc-symbolic-mode t))))))
-		(setq org-tbl-calc-modes (org-table--set-calc-mode
-					  (car mode) (cadr mode))
-		      fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2575,17 +2563,17 @@ location of point."
 	(setq form0 form)
 	;; Insert the references to fields in same row
 	(while (string-match "\\$\\(\\([-+]\\)?[0-9]+\\)" form)
-	  (setq n (+ (string-to-number (match-string 1 form))
-		     (if (match-end 2) n0 0))
-		x (nth (1- (if (= n 0) n0 (max n 1))) fields)
-		formrpl (save-match-data
-			  (org-table-make-reference
-			   x keep-empty numbers lispp)))
-	  (when (or (not x)
-		    (save-match-data
-		      (string-match (regexp-quote formula) formrpl)))
-	    (user-error "Invalid field specifier \"%s\""
-			(match-string 0 form)))
+	  (let* ((n (+ (string-to-number (match-string 1 form))
+		       (if (match-end 2) n0 0)))
+		 (x (nth (1- (if (= n 0) n0 (max n 1))) fields)))
+	    (setq formrpl (save-match-data
+			    (org-table-make-reference
+			     x keep-empty numbers lispp)))
+	    (when (or (not x)
+		      (save-match-data
+			(string-match (regexp-quote formula) formrpl)))
+	      (user-error "Invalid field specifier \"%s\""
+			  (match-string 0 form))))
 	  (setq form (replace-match formrpl t t form)))
 
 	(if lispp
-- 
2.28.0

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

Just to say that I have done a quick test with this and I really like
it.  Simple calculations with units of mol/s and mol/min work perfectly.

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 20/10/2020 15:44, Eric S Fraga wrote:
> Just to say that I have done a quick test with this and I really like
> it.  Simple calculations with units of mol/s and mol/min work perfectly.

Thank you for testing Eric.

To cover the use case of monetary quantities discussed in the other
thread I would like to also add an `m?` (or M?) flag that can combine
the effect of the existing `f?` flag and the new `u` flag. But I don't
know if sparing one character is worth one character in the small flag
characters set.

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

On Tuesday, 20 Oct 2020 at 16:00, Daniele Nicolodi wrote:
> To cover the use case of monetary quantities discussed in the other
> thread I would like to also add an `m?` (or M?) flag that can combine
> the effect of the existing `f?` flag and the new `u` flag. But I don't
> know if sparing one character is worth one character in the small flag
> characters set.

Well, I personally am perfectly fine with uf3 instead of m3 (say) as the
former has a clear intent.

But, as you'll have seen by my followup message, there is something
strange going on with u and f combined.  But I'm still playing.
-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

Hello again,

Following up on myself.  I'm seeing some strange behaviour although unit
calculations are working nicely.  For instance, this table:

#+begin_src org
  | stream   | a            | b            | c            | total        |   x_a |   x_b |   x_c |
  |          | <l>          | <l>          | <l>          | <l>          |   <r> |   <r> |   <r> |
  |----------+--------------+--------------+--------------+--------------+-------+-------+-------|
  | feed     | 1.05 mol/s   | 1.05 mol/s   |              | 2.10 mol / s | 0.500 | 0.500 |     0 |
  | effluent | 0.74 mol / s | 0.74 mol / s | 0.32 mol / s | 1.80 mol / s | 0.411 | 0.411 | 0.178 |
  ,#+TBLFM: $5=vsum($2..$4);uf2::$6=$2/$5;uf3::$7=$3/$5;uf3::$8=$4/$5;uf3::@4$2=(1-0.3)*@-1;uf2::@4$3=(1-0.3)*@-1;uf2::@4$4=@-1+0.3*@-1$-1;uf2
#+end_src

does not seem to pay attention to the f3 mode in the last column, first
data row.

I've also seen some (difficult to replicate) problem with column widths
where the columns are much wider than the expected.  I'll keep playing
to see if I can isolate the column width behaviour.

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 20/10/2020 16:19, Eric S Fraga wrote:
> Hello again,
> 
> Following up on myself.  I'm seeing some strange behaviour although unit
> calculations are working nicely.  For instance, this table:
> 
> #+begin_src org
>   | stream   | a            | b            | c            | total        |   x_a |   x_b |   x_c |
>   |          | <l>          | <l>          | <l>          | <l>          |   <r> |   <r> |   <r> |
>   |----------+--------------+--------------+--------------+--------------+-------+-------+-------|
>   | feed     | 1.05 mol/s   | 1.05 mol/s   |              | 2.10 mol / s | 0.500 | 0.500 |     0 |
>   | effluent | 0.74 mol / s | 0.74 mol / s | 0.32 mol / s | 1.80 mol / s | 0.411 | 0.411 | 0.178 |
>   ,#+TBLFM: $5=vsum($2..$4);uf2::$6=$2/$5;uf3::$7=$3/$5;uf3::$8=$4/$5;uf3::@4$2=(1-0.3)*@-1;uf2::@4$3=(1-0.3)*@-1;uf2::@4$4=@-1+0.3*@-1$-1;uf2
> #+end_src
> 
> does not seem to pay attention to the f3 mode in the last column, first
> data row.

It is something related to how Calc computes the result. The f2 mode
specifies the formatting for floating point values, however it seems
that Calc treats the zero (from the missing value in the fourth column)
divided by a float (from the value in the fifth column) as an integer
and not as a float. This may be because the org substitutes a "0" for
the missing value, thus an integer. Still, I am not sure dividing an
integer by a float should result in an integer (I guess zero is special
cased here).

If you change the formula for that field to:

  #+TBLFM: $8=$4*1.0/$5;uf3

to force the $4 field to be evaluated as a float (there are other ways
to get the same effect) you get the expected result (I think).

> I've also seen some (difficult to replicate) problem with column widths
> where the columns are much wider than the expected.  I'll keep playing
> to see if I can isolate the column width behaviour.

I haven't touched any code dealing with columns width (I believe).

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

On 20/10/2020 16:32, Daniele Nicolodi wrote:
> On 20/10/2020 16:19, Eric S Fraga wrote:
>> Hello again,
>>
>> Following up on myself.  I'm seeing some strange behaviour although unit
>> calculations are working nicely.  For instance, this table:
>>
>> #+begin_src org
>>   | stream   | a            | b            | c            | total        |   x_a |   x_b |   x_c |
>>   |          | <l>          | <l>          | <l>          | <l>          |   <r> |   <r> |   <r> |
>>   |----------+--------------+--------------+--------------+--------------+-------+-------+-------|
>>   | feed     | 1.05 mol/s   | 1.05 mol/s   |              | 2.10 mol / s | 0.500 | 0.500 |     0 |
>>   | effluent | 0.74 mol / s | 0.74 mol / s | 0.32 mol / s | 1.80 mol / s | 0.411 | 0.411 | 0.178 |
>>   ,#+TBLFM: $5=vsum($2..$4);uf2::$6=$2/$5;uf3::$7=$3/$5;uf3::$8=$4/$5;uf3::@4$2=(1-0.3)*@-1;uf2::@4$3=(1-0.3)*@-1;uf2::@4$4=@-1+0.3*@-1$-1;uf2
>> #+end_src
>>
>> does not seem to pay attention to the f3 mode in the last column, first
>> data row.
> 
> It is something related to how Calc computes the result. The f2 mode
> specifies the formatting for floating point values, however it seems
> that Calc treats the zero (from the missing value in the fourth column)
> divided by a float (from the value in the fifth column) as an integer
> and not as a float. This may be because the org substitutes a "0" for
> the missing value, thus an integer. Still, I am not sure dividing an
> integer by a float should result in an integer (I guess zero is special
> cased here).
> 
> If you change the formula for that field to:
> 
>   #+TBLFM: $8=$4*1.0/$5;uf3
> 
> to force the $4 field to be evaluated as a float (there are other ways
> to get the same effect) you get the expected result (I think).

There are other funny Calc behaviors: if an expression results in a
number with an unit where the numerical part is exactly 1, the printed
result looses the numerical part and only the units are printed.

Cheers,
Dan

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

On Tuesday, 20 Oct 2020 at 16:53, Daniele Nicolodi wrote:
> There are other funny Calc behaviors: if an expression results in a
> number with an unit where the numerical part is exactly 1, the printed
> result looses the numerical part and only the units are printed.

Yes, this I'm used to (and it is *annoying* sometimes).  But given what
calc brings to the party, I can put up with small things like this!

Thanks again,
eric

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Eric S Fraga]

On Tuesday, 20 Oct 2020 at 16:32, Daniele Nicolodi wrote:
> This may be because the org substitutes a "0" for the missing value,
> thus an integer. 

This makes sense.  Thank you.

-- 
: Eric S Fraga via Emacs 28.0.50, Org release_9.4-61-ga88806.dirty

Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Daniele Nicolodi]

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

Hello,

after working on this I realized that the org-tbl-calc-modes variables
is used only locally despite being declare globally. Maybe a remnant
from pre-lexical-binding times. Attached is a patch (on top of the
others one in this thread) that simplifies things a little.

Cheers,
Dan


On 20/10/2020 15:30, Daniele Nicolodi wrote:
> Hello,
> 
> attached there are a few patches reworking the code, fixing the bug, and
> introducing a new mode flag to enable Calc's units simplification mode
> as discussed in a recent thread on the mailing list.  I haven't updated
> the documentation.  I can do it once we agree that this feature is a
> good idea.
> 
> Cheers,
> Dan
> 
> 
> On 19/10/2020 17:38, Daniele Nicolodi wrote:
>> Hello,
>>
>> I am hacking org-table-eval-formula (see thread about monetary values in
>> org-tables) which uses this inline function:
>>
>> (defsubst org-table--set-calc-mode (var &optional value)
>>   (if (stringp var)
>>       (setq var (assoc var '(("D" calc-angle-mode deg)
>> 			     ("R" calc-angle-mode rad)
>> 			     ("F" calc-prefer-frac t)
>> 			     ("S" calc-symbolic-mode t)))
>> 	    value (nth 2 var) var (nth 1 var)))
>>   (if (memq var org-tbl-calc-modes)
>>       (setcar (cdr (memq var org-tbl-calc-modes)) value)
>>     (cons var (cons value org-tbl-calc-modes)))
>>   org-tbl-calc-modes)
>>
>> which I am not able to understand or which is not correct.
>>
>> The first (if ...) does some value substitutions, however, IIUC the
>> second (if ...) sets a new value for an entry in the org-tbl-calc-modes
>> plist if the entry is already present and builds a new plist with the
>> entry prepended if the entry is not there. However, the original plist
>> is returned and not the one with the new entry prepended.
>>
>> It does not seem to be the intended behavior.
>>
>> Shouldn't this be simply:
>>
>> (defsubst org-table--set-calc-mode (var &optional value)
>>   (if (stringp var)
>>       (setq var (assoc var '(("D" calc-angle-mode deg)
>> 			     ("R" calc-angle-mode rad)
>> 			     ("F" calc-prefer-frac t)
>> 			     ("S" calc-symbolic-mode t)))
>> 	    value (nth 2 var) var (nth 1 var)))
>>   (plist-put org-tbl-calc-modes var value))
>>
>> or, better, the code refactored to do not use this function?
>>
>> Cheers,
>> Dan
>>
> 


[-- Attachment #2: 0004-org-table-Remove-unused-org-tbl-calc-modes-variable.patch --]
[-- Type: text/plain, Size: 4374 bytes --]

From 2d4521a032ec3e4174c97b2b2e9a08491e9870fb Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Wed, 21 Oct 2020 17:47:15 +0200
Subject: [PATCH 4/4] org-table: Remove unused org-tbl-calc-modes variable

* org-table.el (org-tbl-calc-modes): Remove the variable declaration
as the varialble is only used as a local variable in `org-table-eval-formula'.

* org-table.el (org-table--set-calc-mode): Drop convenience macro.

* org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
local variable without the org-table prefix and usr the gained screen
real estate to avoid indirection through covenience macro.
---
 lisp/org-table.el | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 6b92656bd..1651decd3 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -676,8 +676,6 @@ Will be filled automatically during use.")
     ("_" . "Names for values in row below this one.")
     ("^" . "Names for values in row above this one.")))
 
-(defvar org-tbl-calc-modes nil)
-
 (defvar org-pos nil)
 
 \f
@@ -721,9 +719,6 @@ Field is restored even in case of abnormal exit."
 	 (org-table-goto-column ,column)
 	 (set-marker ,line nil)))))
 
-(defsubst org-table--set-calc-mode (var value)
-  (setq org-tbl-calc-modes (plist-put org-tbl-calc-modes var value)))
-
 \f
 ;;; Predicates
 
@@ -2424,7 +2419,7 @@ location of point."
 			equation
 		      (org-table-get-formula equation (equal arg '(4)))))
 	   (n0 (org-table-current-column))
-	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
+	   (calc-modes (copy-sequence org-calc-default-modes))
 	   (numbers nil)	   ; was a variable, now fixed default
 	   (keep-empty nil)
 	   form form0 formrpl formrg bw fmt ev orig lispp literal
@@ -2440,11 +2435,11 @@ location of point."
 	      (let ((c (string-to-char (match-string 1 fmt)))
 		    (n (string-to-number (match-string 2 fmt))))
 		(cl-case c
-		  (?p (org-table--set-calc-mode 'calc-internal-prec n))
-		  (?n (org-table--set-calc-mode 'calc-float-format (list 'float n)))
-		  (?f (org-table--set-calc-mode 'calc-float-format (list 'fix n)))
-		  (?s (org-table--set-calc-mode 'calc-float-format (list 'sci n)))
-		  (?e (org-table--set-calc-mode 'calc-float-format (list 'eng n)))))
+		  (?p (setf (cl-getf calc-modes 'calc-internal-prec) n))
+		  (?n (setf (cl-getf calc-modes 'calc-float-format) (list 'float n)))
+		  (?f (setf (cl-getf calc-modes 'calc-float-format) (list 'fix n)))
+		  (?s (setf (cl-getf calc-modes 'calc-float-format) (list 'sci n)))
+		  (?e (setf (cl-getf calc-modes 'calc-float-format) (list 'eng n)))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
@@ -2452,16 +2447,16 @@ location of point."
 		(cl-case c
 		  (?t (setq duration t numbers t
 		      	    duration-output-format org-table-duration-custom-format))
-		  (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
+		  (?u (setf (cl-getf calc-modes 'calc-simplify-mode) 'units))
 		  (?T (setq duration t numbers t duration-output-format nil))
 		  (?U (setq duration t numbers t duration-output-format 'hh:mm))
 		  (?N (setq numbers t))
 		  (?L (setq literal t))
 		  (?E (setq keep-empty t))
-		  (?D (org-table--set-calc-mode 'calc-angle-mode 'deg))
-		  (?R (org-table--set-calc-mode 'calc-angle-mode 'rad))
-		  (?F (org-table--set-calc-mode 'calc-prefer-frac t))
-		  (?S (org-table--set-calc-mode 'calc-symbolic-mode t))))
+		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
+		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
+		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (unless (string-match "\\S-" fmt)
@@ -2606,7 +2601,7 @@ location of point."
 
 	  (setq ev (if (and duration (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" form))
 		       form
-		     (calc-eval (cons form org-tbl-calc-modes)
+		     (calc-eval (cons form calc-modes)
 				(when (and (not keep-empty) numbers) 'num)))
 		ev (if duration (org-table-time-seconds-to-string
 				 (if (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" ev)
-- 
2.28.0