Re: ob-octave: improve MATLAB support

[John C <john.ciolfi.32@gmail.com>]

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

Hi

Thanks for the feedback. I incorporated some of your suggestions for
org-mode. I'll add the examples to https://git.sr.ht/~bzg/worg once
the changes become part of org-mode.

Regarding (defvar org-babel-default-header-args:matlab '((:session .
"*MATLAB*")), this was done on purpose. We've been using this setting
for a while. Without it, org matlab evaluation is not good. I added a
comment as to why it's there.

I looked into the use of REMOVE-ECHO which ob-octave.el already sets
to t and don't believe this will help us. I think low-level comit
requires the input which is captured by org.

Any guidance on adding a test? To test properly we'll need MATLAB and
https://github.com/mathworks/Emacs-MATLAB-Mode. Note, in
https://github.com/mathworks/Emacs-MATLAB-Mode, I have a test for all
this. Currently Emacs-MATLAB-Mode is advising org to get things
working. When org is updated, Emacs-MATLAB-Mode will automatically not
advise org.

See updated attach patch.

I have yet to hear back from FSF on the copyright assignment. I'm
happy to sign this, so if you can help, that would be good.

Thanks
John

On Sat, Nov 9, 2024 at 4:31 AM Ihor Radchenko <yantar92@posteo.net> wrote:
>
> John C <john.ciolfi.32@gmail.com> writes:
>
> > Please see the attached patch which makes MATLAB work with org babel
> > eval. Here's the commit message:
>
> Thanks a lot for the patch!
>
> The patch introduces major changes. Would it be possible to add some
> tests as well?
>
> > Note, I have sent in the paper work for the FSF copyright assignment.
>
> Let us know when it is done. Also, if there are any problems or delays
> in the process, we can try helping.
>
>
> My initial comments on the patch are below.
>
> > +*** ob-octave: improved MATLAB support
>
> "fixed" maybe :)
>
> > +Improved ob-octave MATLAB (https://www.mathworks.com) code blocks processing.
> > +The prior version of ob-octave didn't correctly support MATLAB.  To use MATLAB
> > +with org, you need https://github.com/MathWorks/Emacs-MATLAB-Mode.
> > +
> > +Example ~verbatim~ code block:
> > +
> > +#+begin_src org
> > +,#+begin_src matlab :results verbatim
>
> I am not sure if examples are necessary in the NEWS. Maybe we can simply
> mention that :results verbatim, output, and latex are supported now.
>
> As for examples, please put them into
> https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-octave-matlab.html
> The source code for that page is at https://git.sr.ht/~bzg/worg
>
> > -;; Matlab
> > -
> > -;; matlab.el required for interactive emacs sessions and matlab-mode
> > -;; major mode for source code editing buffer
> > -;; https://matlab-emacs.sourceforge.net/
> > +;; 1) MATLAB from https://www.mathworks.com
>
> Please remove all the links to mathworks.com - it is FSF policy to not
> link to proprietary software pages. We cannot have these links in Org
> code.
>
> > +;; 2) https://github.com/mathworks/Emacs-MATLAB-Mode
>
> This one is GPL-licensed and hence fine.
>
> > -(defvar org-babel-default-header-args:matlab '())
> > +;; Use the session "*MATLAB*" buffer created by `matlab-shell` for code evaluation.
> > +(defvar org-babel-default-header-args:matlab '((:session . "*MATLAB*")))
>
> The new default implies that sessions will be enabled by default. I am
> not sure if it is what you intended to do.
>
> > -(defvar org-babel-matlab-with-emacs-link nil
> > -  "If non-nil use matlab-shell-run-region for session evaluation.
> > -This will use EmacsLink if (matlab-with-emacs-link) evaluates
> > -to a non-nil value.")
> > -
> > -(defvar org-babel-matlab-emacs-link-wrapper-method
> > -  "%s
> > -if ischar(ans), fid = fopen('%s', 'w'); fprintf(fid, '%%s\\n', ans); fclose(fid);
> > -else, save -ascii %s ans
> > -end
> > -delete('%s')
>
> I understand that these variable no longer make sense after we remove
> support of MATLAB Emacs Link. However, I'd prefer to not remove
> variables without warning. Instead, please deprecate them.
>
> > +  (when matlabp
> > +    ;; When we send multi-line input to `matlab-shell', we'll see the "body"
> > +    ;; code lines echoed in the output which is not what one would expect.  To
>
> Maybe you can somehow make use of REMOVE-ECHO in `org-babel-comint-with-output'?
>
> --
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>

[-- Attachment #2: ob-octave.el.patch --]
[-- Type: text/x-patch, Size: 15466 bytes --]

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 5d421172f..5513c172d 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -162,6 +162,13 @@ bibliography format requires them to be written in title-case.
 
 # This also includes changes in function behavior from Elisp perspective.
 
+*** ob-octave: fixed MATLAB support
+
+Fixed ob-octave MATLAB (https://www.mathworks.com) code blocks processing.  The prior version of
+ob-octave didn't correctly support MATLAB.  To use MATLAB with org, you need
+https://github.com/MathWorks/Emacs-MATLAB-Mode. Now ~#+begin_src matlab~ code blocks with ~:results
+verbatim~, ~:results output~, ~:results output latex~, or ~:results file graphics~ are supported.
+
 *** ob-sqlite: Added ability to open a database in readonly mode
 
 Added option :readonly to ob-sqlite.
diff --git a/lisp/ob-matlab.el b/lisp/ob-matlab.el
index de8deadbe..083dcdec3 100644
--- a/lisp/ob-matlab.el
+++ b/lisp/ob-matlab.el
@@ -28,11 +28,10 @@
 
 ;;; Requirements:
 
-;; Matlab
-
-;; matlab.el required for interactive emacs sessions and matlab-mode
-;; major mode for source code editing buffer
-;; https://matlab-emacs.sourceforge.net/
+;; 1) MATLAB
+;; 2) https://github.com/mathworks/Emacs-MATLAB-Mode
+;;    For matlab-shell to run MATLAB within Emacs and matlab-mode
+;;    major mode for source code editing buffer
 
 ;;; Code:
 
diff --git a/lisp/ob-octave.el b/lisp/ob-octave.el
index 005990f20..5ac1de1e4 100644
--- a/lisp/ob-octave.el
+++ b/lisp/ob-octave.el
@@ -1,4 +1,4 @@
-;;; ob-octave.el --- Babel Functions for Octave and Matlab -*- lexical-binding: t; -*-
+;;; ob-octave.el --- Babel Functions for Octave and MATLAB -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 2010-2024 Free Software Foundation, Inc.
 
@@ -30,6 +30,8 @@
 
 ;;; Code:
 
+(require 'cl-seq)
+
 (require 'org-macs)
 (org-assert-version)
 
@@ -39,7 +41,52 @@
 (declare-function matlab-shell "ext:matlab-mode")
 (declare-function matlab-shell-run-region "ext:matlab-mode")
 
-(defvar org-babel-default-header-args:matlab '())
+;; With `org-babel-default-header-args:matlab' set to
+;;  '((:session . "*MATLAB*")))
+;; each matlab code block evaluation will reuse the "*MATLAB*" buffer
+;; created by `matlab-shell' for code evaluation.  The benefit of this
+;; is that evaluation is very fast for evaluations after the first
+;; evaluation. The first evaluation may be slower because it can take
+;; a long time to start MATLAB.  Reusing the session buffer means
+;; state is maintained between evaluations.  To avoid reuse of state,
+;; you can clear the MATLAB workspace.  This setting aligns with
+;; typical MATLAB use, where MATLAB is started and used for a long
+;; period of period of time.  Another benefit of this setting is that
+;; you can see the history of the evaluations in the "*MATLAB*"
+;; command window buffer.  For example:
+;;
+;;  #+begin_src matlab :results output
+;;    a = 123
+;;  #+end_src
+;;
+;;  #+RESULTS:
+;;  : a =
+;;  :
+;;  :    123
+;;
+;;  #+begin_src matlab :results output
+;;    b = a * 2
+;;  #+end_src
+;;
+;;  #+RESULTS:
+;;  : b =
+;;  :
+;;  :    246
+;;
+;;  #+begin_src matlab :results output
+;;    clear
+;;    c = b * 2
+;;  #+end_src
+;;
+;;  #+RESULTS:
+;;  : Unrecognized function or variable 'b'.
+;;
+;; If you want a new session each time you evaluate a MATLAB code block,
+;;   (setq 'org-babel-default-header-args:matlab '())
+;; However, this will make evaluations slower and is not typically how
+;; people use MATLAB.
+(defvar org-babel-default-header-args:matlab '((:session . "*MATLAB*")))
+
 (defvar org-babel-default-header-args:octave '())
 
 (defvar org-babel-matlab-shell-command "matlab -nosplash"
@@ -47,18 +94,42 @@
 (defvar org-babel-octave-shell-command "octave -q"
   "Shell command to run octave as an external process.")
 
-(defvar org-babel-matlab-with-emacs-link nil
-  "If non-nil use matlab-shell-run-region for session evaluation.
-This will use EmacsLink if (matlab-with-emacs-link) evaluates
-to a non-nil value.")
-
-(defvar org-babel-matlab-emacs-link-wrapper-method
-  "%s
-if ischar(ans), fid = fopen('%s', 'w'); fprintf(fid, '%%s\\n', ans); fclose(fid);
-else, save -ascii %s ans
-end
-delete('%s')
+(make-obsolete-variable 'org-babel-matlab-with-emacs-link
+                        "MATLAB removed EmacsLink in R2009a." "2009")
+
+(make-obsolete-variable 'org-babel-matlab-emacs-link-wrapper-method
+                        "MATLAB removed EmacsLink in R2009a." "2009")
+
+(defvar org-babel-matlab-print "print(\"-dpng\", %S);\nans=%S;"
+  ;; MATLAB command-function duality requires that the file name be specified
+  ;; without quotes. Using: print -dpng "file.png", would produce a file with
+  ;; the quotes in the file name on disk. Therefore, use the functional form
+  ;; to handle files with spaces, print("-dpng", "file.png").
+  ;; Example:
+  ;;    #+begin_src matlab :file "sine wave.png" :results file graphics
+  ;;      t = [0 : 0.1 : 2*pi];
+  ;;      y = sin(t);
+  ;;      plot(t, y);
+  ;;      set(gcf, 'PaperUnits', 'inches', 'PaperPosition', [0 0 4 3]) % Set the size to 4" x 3"
+  ;;    #+end_src
+  ;;
+  ;;    #+RESULTS:
+  ;;    [[file:sine wave.png]]
+  "MATLAB format specifier to print current figure to a file.")
+
+(defvar org-babel-octave-print "print -dpng %S\nans=%S"
+  "Octave format specifier to print current figure to a file.")
+
+(defvar org-babel-matlab-wrapper-method
+  (concat "\
+cd('%s');
+%s
+if ~exist('ans', 'var') ans = ''; end; \
+writematrix(ans, '%s', 'Delimiter', 'tab');
 ")
+  "Format specifier used when evaluating MATLAB code blocks.
+Arguments are the `default-directory', the MATLAB code, and a result file.txt.")
+
 (defvar org-babel-octave-wrapper-method
   "%s
 if ischar(ans), fid = fopen('%s', 'w'); fdisp(fid, ans); fclose(fid);
@@ -92,7 +163,10 @@ When MATLABP is non-nil, execute Matlab.  Otherwise, execute Octave."
 				 (list
 				  "set (0, \"defaultfigurevisible\", \"off\");"
 				  full-body
-				  (format "print -dpng %S\nans=%S" gfx-file gfx-file))
+				  (format (if matlabp
+                                              org-babel-matlab-print
+                                            org-babel-octave-print)
+                                          gfx-file gfx-file))
 				 "\n")
 		    full-body)
 		  result-type matlabp)))
@@ -153,6 +227,23 @@ If there is not a current inferior-process-buffer in SESSION then
 create.  Return the initialized session.  PARAMS are src block parameters."
   (org-babel-octave-initiate-session session params 'matlab))
 
+(defun org-babel-matlab-shell ()
+  "Start and/or wait for MATLAB shell."
+  (require 'matlab-shell) ;; make `matlab-shell-busy-checker' available
+  (cond
+   ((fboundp 'matlab-shell-busy-checker)
+    ;; Start the shell if needed.  `matlab-shell' will reuse existing if already running.
+    (matlab-shell)
+    ;; If we just started the matlab-shell, wait for the prompt.  If we do not
+    ;; wait, then the startup messages will show up in the evaluation results.
+    (matlab-shell-busy-checker 'wait-for-prompt))
+   (t
+    (message (concat "You version of matlab-mode is old.\n"
+                     "Please update, see https://github.com/mathworks/Emacs-MATLAB-Mode\n"
+                     "Updating will eliminate unexpected output in your results\n"))
+    (sit-for 3)
+    (matlab-shell))))
+
 (defun org-babel-octave-initiate-session (&optional session _params matlabp)
   "Create an octave inferior process buffer.
 If there is not a current inferior-process-buffer in SESSION then
@@ -165,9 +256,15 @@ Octave session, unless MATLABP is non-nil."
   (unless (string= session "none")
     (let ((session (or session
 		       (if matlabp "*Inferior Matlab*" "*Inferior Octave*"))))
-      (if (org-babel-comint-buffer-livep session) session
+      (if (org-babel-comint-buffer-livep session)
+          (progn
+            (when (and matlabp (fboundp 'matlab-shell-busy-checker))
+              ;; Can't evaluate if the matlab-shell is currently running code
+              (matlab-shell-busy-checker 'error-if-busy))
+            session)
 	(save-window-excursion
-	  (if matlabp (unless org-babel-matlab-with-emacs-link (matlab-shell))
+	  (if matlabp
+              (org-babel-matlab-shell)
 	    (run-octave))
 	  (rename-buffer (if (bufferp session) (buffer-name session)
 			   (if (stringp session) session (buffer-name))))
@@ -183,66 +280,111 @@ value of the last statement in BODY, as elisp."
       (org-babel-octave-evaluate-session session body result-type matlabp)
     (org-babel-octave-evaluate-external-process body result-type matlabp)))
 
+(defun org-babel-octave-wrapper-tmp-file (matlabp)
+  "Return a local tmp file with name adjusted for MATLABP."
+  (if matlabp
+      ;; writematrix requires a file ending with '.txt'
+      (org-babel-temp-file "matlab-" ".txt")
+    (org-babel-temp-file "octave-")))
+
+(defun org-babel-octave-get-code-to-eval (body tmp-file matlabp)
+  "Format BODY of the code block for evaluation saving results to TMP-FILE.
+If MATLABP, format for MATLAB, else format for Octave."
+    (if matlabp
+        (format org-babel-matlab-wrapper-method default-directory body tmp-file)
+      (format org-babel-octave-wrapper-method body tmp-file tmp-file)))
+
 (defun org-babel-octave-evaluate-external-process (body result-type matlabp)
-  "Evaluate BODY in an external Octave or Matalab process.
+  "Evaluate BODY in an external Octave or MATLAB process.
 Process the result as RESULT-TYPE.  Use Octave, unless MATLABP is non-nil."
   (let ((cmd (if matlabp
 		 org-babel-matlab-shell-command
 	       org-babel-octave-shell-command)))
     (pcase result-type
       (`output (org-babel-eval cmd body))
-      (`value (let ((tmp-file (org-babel-temp-file "octave-")))
+      (`value (let ((tmp-file (org-babel-process-file-name
+                               (org-babel-octave-wrapper-tmp-file matlabp)
+                               'noquote)))
 	        (org-babel-eval
 		 cmd
-		 (format org-babel-octave-wrapper-method body
-			 (org-babel-process-file-name tmp-file 'noquote)
-			 (org-babel-process-file-name tmp-file 'noquote)))
+                 (org-babel-octave-get-code-to-eval body tmp-file matlabp))
 	        (org-babel-octave-import-elisp-from-file tmp-file))))))
 
+(defun org-babel-body-for-output (body matlabp)
+  "If MATLABP, fixup BODY for MATLAB output result-type."
+  (when matlabp
+    ;; When we send multi-line input to `matlab-shell', we'll see the "body"
+    ;; code lines echoed in the output which is not what one would expect.  To
+    ;; remove these unwanted lines, we append a comment "%-<org-eval>" to each
+    ;; line in the body MATLAB code.  After we collect the results from
+    ;; evaluation, we leverage the "%-<org-eval>" to remove the unwanted lines.
+    ;; Example of desired behavior:
+    ;;     #+begin_src matlab :results output
+    ;;       disp('The results are:')
+    ;;       a = [1, 2; 3, 4]
+    ;;       b = a * 2
+    ;;     #+end_src
+    ;;
+    ;;     #+RESULTS:
+    ;;     #+begin_example
+    ;;     The results are:
+    ;;
+    ;;     a =
+    ;;
+    ;;          1     2
+    ;;          3     4
+    ;;
+    ;;     b =
+    ;;
+    ;;          2     4
+    ;;          6     8
+    ;;     #+end_example
+
+    (setq body (replace-regexp-in-string "\n" " %-<org-eval>-\n" body))
+    (when (not (string-match "\n\\'" body))
+      (setq body (concat body " %-<org-eval>-"))))
+  body)
+
+(defun org-babel-fix-up-output (results matlabp)
+  "Fix up RESULTS for output result-type when MATLABP."
+  ;; When we send multi-line input to `matlab-shell', we'll see the "body" code
+  ;; lines echoed in the output.  Therefore, leverage the "%-<org-eval>" to
+  ;; remove the unnecessary lines.
+  (if matlabp
+      (let ((fixed-results (replace-regexp-in-string "^[^\n]*%-<org-eval>-\n" ""
+                                                     results)))
+        ;; Remove unnecessary starting blank line caused by stripping %-<org-eval>
+        (setq fixed-results (replace-regexp-in-string "\\`[[:space:]\r\n]+" "" fixed-results))
+        ;; matlab-shell wraps errors in <ERRORTXT> and </ERRORTXT> so remove those
+        (replace-regexp-in-string "</?ERRORTXT>[\r\n]" "" fixed-results))
+    results))
+
 (defun org-babel-octave-evaluate-session
     (session body result-type &optional matlabp)
   "Evaluate BODY in SESSION."
-  (let* ((tmp-file (org-babel-temp-file (if matlabp "matlab-" "octave-")))
-	 (wait-file (org-babel-temp-file "matlab-emacs-link-wait-signal-"))
+  (let* ((tmp-file (org-babel-octave-wrapper-tmp-file matlabp))
 	 (full-body
 	  (pcase result-type
 	    (`output
 	     (mapconcat
 	      #'org-babel-chomp
-	      (list body org-babel-octave-eoe-indicator) "\n"))
+	      (list (org-babel-body-for-output body matlabp)
+                    org-babel-octave-eoe-indicator)
+              "\n"))
 	    (`value
-	     (if (and matlabp org-babel-matlab-with-emacs-link)
-		 (concat
-		  (format org-babel-matlab-emacs-link-wrapper-method
-			  body
-			  (org-babel-process-file-name tmp-file 'noquote)
-			  (org-babel-process-file-name tmp-file 'noquote) wait-file) "\n")
-	       (mapconcat
-		#'org-babel-chomp
-		(list (format org-babel-octave-wrapper-method
-			      body
-			      (org-babel-process-file-name tmp-file 'noquote)
-			      (org-babel-process-file-name tmp-file 'noquote))
-		      org-babel-octave-eoe-indicator) "\n")))))
-	 (raw (if (and matlabp org-babel-matlab-with-emacs-link)
-		  (save-window-excursion
-		    (with-temp-buffer
-		      (insert full-body)
-		      (write-region "" 'ignored wait-file nil nil nil 'excl)
-		      (matlab-shell-run-region (point-min) (point-max))
-		      (message "Waiting for Matlab Emacs Link")
-		      (while (file-exists-p wait-file) (sit-for 0.01))
-		      "")) ;; matlab-shell-run-region doesn't seem to
-		;; make *matlab* buffer contents easily
-		;; available, so :results output currently
-		;; won't work
-		(org-babel-comint-with-output
-		    (session
-		     (if matlabp
-			 org-babel-octave-eoe-indicator
-		       org-babel-octave-eoe-output)
-		     t full-body)
-		  (insert full-body) (comint-send-input nil t))))
+	     (mapconcat
+	      #'org-babel-chomp
+	      (list (org-babel-octave-get-code-to-eval body tmp-file matlabp)
+		    org-babel-octave-eoe-indicator)
+              "\n"))))
+	 (raw-results
+	  (org-babel-comint-with-output
+	      (session
+	       (if matlabp
+		   org-babel-octave-eoe-indicator
+		 org-babel-octave-eoe-output)
+	       t full-body)
+	    (insert full-body) (comint-send-input nil t)))
 	 results)
     (pcase result-type
       (`value
@@ -250,12 +392,13 @@ Process the result as RESULT-TYPE.  Use Octave, unless MATLABP is non-nil."
       (`output
        (setq results
 	     (if matlabp
-		 (cdr (reverse (delete "" (mapcar #'org-strip-quotes
-					          (mapcar #'org-trim raw)))))
+                 (cdr (reverse (delete "" (mapcar #'org-strip-quotes
+					          (mapcar #'org-trim raw-results)))))
 	       (cdr (member org-babel-octave-eoe-output
 			    (reverse (mapcar #'org-strip-quotes
-					     (mapcar #'org-trim raw)))))))
-       (mapconcat #'identity (reverse results) "\n")))))
+					     (mapcar #'org-trim raw-results)))))))
+       (org-babel-fix-up-output
+        (mapconcat #'identity (reverse results) "\n") matlabp)))))
 
 (defun org-babel-octave-import-elisp-from-file (file-name)
   "Import data from FILE-NAME.