[PATCH] ob-python: support header argument `:results file graphics'

[PATCH] ob-python: support header argument `:results file graphics'

[Liu Hui]

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

Hi,

This patch adds graphics output support for ob-python via matplotlib.
Specifically, it allows to use header argument `:results file
graphics' as follows:

#+begin_src python :file "test.png" :results graphics file
import matplotlib.pyplot as plt
plt.plot([1,2,3,4,5])
#+end_src

The feature is described in the documentation as follows and has been
supported by ob-R, ob-julia, etc.

> ‘graphics’
>      When used along with ‘file’ type, the result is a link to the file
>      specified in ‘:file’ header argument.

[-- Attachment #2: 0001-ob-python-support-header-argument-results-file-graph.patch --]
[-- Type: text/x-patch, Size: 2311 bytes --]

From ee8d236310cd4bde4e33610b2b794022861fbd6a Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Mon, 3 Jul 2023 11:13:58 +0800
Subject: [PATCH] ob-python: support header argument `:results file graphics'

* lisp/ob-python.el (org-babel-execute:python): Save current figure to
file when the result type is `file graphics'.
(org-babel-python-save-graphics): New helper function to generate code
to save graphics.
---
 lisp/ob-python.el | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 0e0539d7a..9fae49a4d 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -72,6 +72,8 @@ (defun org-babel-execute:python (body params)
 		   (cdr (assq :session params))))
          (result-params (cdr (assq :result-params params)))
          (result-type (cdr (assq :result-type params)))
+         (graphics-file (and (member "graphics" result-params)
+                             (org-babel-graphical-output-file params)))
 	 (return-val (when (eq result-type 'value)
 		       (cdr (assq :return params))))
 	 (preamble (cdr (assq :preamble params)))
@@ -81,6 +83,8 @@ (defun org-babel-execute:python (body params)
 	   (org-babel-expand-body:generic
 	    body params
 	    (org-babel-variable-assignments:python params))
+           (when graphics-file
+             (org-babel-python-save-graphics graphics-file params))
 	   (when return-val
 	     (format (if session "\n%s" "\nreturn %s") return-val))))
          (result (org-babel-python-evaluate
@@ -149,6 +153,21 @@ (defun org-babel-python-table-or-string (results)
                 res)
       res)))
 
+(defun org-babel-python-save-graphics (out-file _)
+  "Return the code for saving graphics to `OUT-FILE'."
+  (format "
+try:
+    import matplotlib.pyplot
+    if len(matplotlib.pyplot.get_fignums()) > 0:
+        matplotlib.pyplot.gcf().savefig('%s')
+    else:
+        raise RuntimeError('No figure is found. You need to use matplotlib\
+ functions, such as plot and imshow, to produce the graphics.')
+except ModuleNotFoundError:
+    raise RuntimeError('ob-python depends on matplotlib for saving graphics')
+"
+          out-file))
+
 (defvar org-babel-python-buffers '((:default . "*Python*")))
 
 (defun org-babel-python-session-buffer (session)
-- 
2.25.1

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

Liu Hui <liuhui1610@gmail.com> writes:

> This patch adds graphics output support for ob-python via matplotlib.
> Specifically, it allows to use header argument `:results file
> graphics' as follows:
>
> #+begin_src python :file "test.png" :results graphics file
> import matplotlib.pyplot as plt
> plt.plot([1,2,3,4,5])
> #+end_src

This might be a useful feature, but it will break the existing
conventions, if used as in the patch. Currently, the above combination
of :file and :results is treated as the following:

‘graphics’
     When used along with ‘file’ type, the result is a link to the file
     specified in ‘:file’ header argument.  However, unlike plain ‘file’
     type, code block output is not written to the disk.  The block is
     expected to generate the file by its side-effects only, as in the
     following example:

          #+begin_src shell :results file link :file "org-mode-unicorn.svg"
            wget -c "https://orgmode.org/resources/img/org-mode-unicorn.svg"
          #+end_src

          #+RESULTS:
          [[file:org-mode-unicorn.svg]]

     If ‘:file’ header argument is omitted, interpret source block
     result as the file path.

> +try:
> +    import matplotlib.pyplot
> +    if len(matplotlib.pyplot.get_fignums()) > 0:
> +        matplotlib.pyplot.gcf().savefig('%s')
> +    else:
> +        raise RuntimeError('No figure is found. You need to use matplotlib\
> + functions, such as plot and imshow, to produce the graphics.')
> +except ModuleNotFoundError:
> +    raise RuntimeError('ob-python depends on matplotlib for saving graphics')

What if the user wants to use, for example, pyplot instead of
matplotlib?

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Liu Hui]

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

Ihor Radchenko <yantar92@posteo.net> 于2023年7月3日周一 17:28写道：

> This might be a useful feature, but it will break the existing
> conventions, if used as in the patch. Currently, the above combination
> of :file and :results is treated as the following:
>
> ‘graphics’
>      When used along with ‘file’ type, the result is a link to the file
>      specified in ‘:file’ header argument.  However, unlike plain ‘file’
>      type, code block output is not written to the disk.  The block is
>      expected to generate the file by its side-effects only, as in the
>      following example:
>
>           #+begin_src shell :results file link :file "org-mode-unicorn.svg"
>             wget -c "https://orgmode.org/resources/img/org-mode-unicorn.svg"
>           #+end_src
>
>           #+RESULTS:
>           [[file:org-mode-unicorn.svg]]
>
>      If ‘:file’ header argument is omitted, interpret source block
>      result as the file path.

I have updated the patch and ‘:file’ header argument can be omitted
now, e.g.

#+begin_src python :results graphics file
import matplotlib.pyplot as plt
plt.plot([1,2,3,4,5])
plt.savefig('test.png')
return 'test.png'
#+end_src

In this case, `graphics' can be removed too.

> What if the user wants to use, for example, pyplot instead of
> matplotlib?

pyplot is a module of matplotlib. If you mean users want to use other
graphics libraries, they may advise `org-babel-python-save-graphics'
if they want to use `graphics' in the header argument. If necessary,
ob-python may support such libraries too.

[-- Attachment #2: 0001-ob-python-support-header-argument-results-file-graph.patch --]
[-- Type: text/x-patch, Size: 2360 bytes --]

From 8ea69d66552ee0217b7f5c8089b91424b92c3b5a Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Mon, 3 Jul 2023 11:13:58 +0800
Subject: [PATCH] ob-python: support header argument `:results file graphics'

* lisp/ob-python.el (org-babel-execute:python): Save current figure to
file when the result type is `file graphics'.
(org-babel-python-save-graphics): New helper function to generate code
to save graphics.
---
 lisp/ob-python.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 0e0539d7a..6ee52313c 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -72,6 +72,9 @@ (defun org-babel-execute:python (body params)
 		   (cdr (assq :session params))))
          (result-params (cdr (assq :result-params params)))
          (result-type (cdr (assq :result-type params)))
+         (graphics-file (and (member "graphics" result-params)
+                             (ignore-errors
+                               (org-babel-graphical-output-file params))))
 	 (return-val (when (eq result-type 'value)
 		       (cdr (assq :return params))))
 	 (preamble (cdr (assq :preamble params)))
@@ -81,6 +84,8 @@ (defun org-babel-execute:python (body params)
 	   (org-babel-expand-body:generic
 	    body params
 	    (org-babel-variable-assignments:python params))
+           (when graphics-file
+             (org-babel-python-save-graphics graphics-file params))
 	   (when return-val
 	     (format (if session "\n%s" "\nreturn %s") return-val))))
          (result (org-babel-python-evaluate
@@ -149,6 +154,21 @@ (defun org-babel-python-table-or-string (results)
                 res)
       res)))
 
+(defun org-babel-python-save-graphics (out-file _)
+  "Return the code for saving graphics to `OUT-FILE'."
+  (format "
+try:
+    import matplotlib.pyplot
+    if len(matplotlib.pyplot.get_fignums()) > 0:
+        matplotlib.pyplot.gcf().savefig('%s')
+    else:
+        raise RuntimeError('No figure is found. You need to use matplotlib\
+ functions, such as plot and imshow, to produce the graphics.')
+except ModuleNotFoundError:
+    raise RuntimeError('ob-python depends on matplotlib for saving graphics')
+"
+          out-file))
+
 (defvar org-babel-python-buffers '((:default . "*Python*")))
 
 (defun org-babel-python-session-buffer (session)
-- 
2.25.1

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

Liu Hui <liuhui1610@gmail.com> writes:

> I have updated the patch and ‘:file’ header argument can be omitted
> now, e.g.
>
> #+begin_src python :results graphics file
> import matplotlib.pyplot as plt
> plt.plot([1,2,3,4,5])
> plt.savefig('test.png')
> return 'test.png'
> #+end_src

This already works, even without the patch.

> In this case, `graphics' can be removed too.

May you elaborate?

#+begin_src python :results file
implies that the result of execution is file _contents_.

>> What if the user wants to use, for example, pyplot instead of
>> matplotlib?
>
> pyplot is a module of matplotlib. If you mean users want to use other
> graphics libraries, they may advise `org-babel-python-save-graphics'
> if they want to use `graphics' in the header argument. If necessary,
> ob-python may support such libraries too.

This is not acceptable. If src blocks worked in the past, they should
continue working without adjustments in user' setup. We can only break
backwards-compatibility when there is a strong reason to do so. Adding a
new feature is not such a reason.

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Liu Hui]

Ihor Radchenko <yantar92@posteo.net> 于2023年7月3日周一 19:41写道：

> This already works, even without the patch.

Yes. It means the patch doesn't break current usage.

> > In this case, `graphics' can be removed too.
>
> May you elaborate?
>
> #+begin_src python :results file
> implies that the result of execution is file _contents_.

It is consistent with current behavior, e.g. `:results file' is used
to produce graphics file
https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-python.html

> This is not acceptable. If src blocks worked in the past, they should
> continue working without adjustments in user' setup. We can only break
> backwards-compatibility when there is a strong reason to do so. Adding a
> new feature is not such a reason.

Any src blocks worked in the past still work with this patch, without
any adjustments. The patch only makes the following src block, which
cannot produce `test.png' in the past, can produce `test.png'.

#+begin_src python :file "test.png" :results graphics file
import matplotlib.pyplot as plt
plt.plot([1,2,3,4,5])
#+end_src

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

[ CCing ob-python maintainer. ]

Liu Hui <liuhui1610@gmail.com> writes:

>> May you elaborate?
>>
>> #+begin_src python :results file
>> implies that the result of execution is file _contents_.
>
> It is consistent with current behavior, e.g. `:results file' is used
> to produce graphics file
> https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-python.html

WORG documentation is not accurate here.

More precisely,

#+name: some-name
#+begin_src python :results file :file-ext txt

will interpret results as file contents of "some-name.txt".

If the block has :file parameter _or_ :file-exp parameter and #+name,
the results of evaluation are inserted into a file. Otherwise, results
of evaluation are interpreted as file name--*undocumented*.

That said, your patch should still work fine even with these
considerations. But we may need to sort out this undocumented behaviour.
Probably, an error like in `org-babel-graphical-output-file' should be
thrown by `org-babel-generate-file-param'.

> Any src blocks worked in the past still work with this patch, without
> any adjustments. The patch only makes the following src block, which
> cannot produce `test.png' in the past, can produce `test.png'.
>
> #+begin_src python :file "test.png" :results graphics file
> import matplotlib.pyplot as plt
> plt.plot([1,2,3,4,5])
> #+end_src

Right.

What about

#+begin_src python :file "test.png" :results graphics file
import matplotlib.pyplot as plt
plt.plot([1,2,3,4,5])
plt.savefig('test.png')
plt.plot([5,4,3,2,1])
plt.show()
#+end_src

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> WORG documentation is not accurate here.
>
> If the block has :file parameter _or_ :file-exp parameter and #+name,
> the results of evaluation are inserted into a file. Otherwise, results
> of evaluation are interpreted as file name--*undocumented*.
>
> That said, your patch should still work fine even with these
> considerations. But we may need to sort out this undocumented behaviour.
> Probably, an error like in `org-babel-graphical-output-file' should be
> thrown by `org-babel-generate-file-param'.

I think the Worg documentation is accurately describing the behavior of
ob-python -- it's just that ob-python uses ":results file" in a
nonstandard way.

But I'm hesitant to make a breaking change to ob-python on this. Until
now it has been the only way for ob-python to return a plot. And the
matplotlib example in Worg has been there for 10 years, long before I
started maintaining ob-python.

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

Jack Kamm <jackkamm@gmail.com> writes:

> I think the Worg documentation is accurately describing the behavior of
> ob-python -- it's just that ob-python uses ":results file" in a
> nonstandard way.

May you please point me to the place in ob-python where :results file is
specially treated?

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Jack Kamm <jackkamm@gmail.com> writes:
>
>> I think the Worg documentation is accurately describing the behavior of
>> ob-python -- it's just that ob-python uses ":results file" in a
>> nonstandard way.
>
> May you please point me to the place in ob-python where :results file is
> specially treated?

ob-python doesn't treat :results file specially.  So "nonstandard" may
not be the right term.

In fact, :results file also works this way for other Babel languages.
And I used this behavior before for plotting with ob-reticulate blocks.

I attach a patch to fix the documentation in the manual about this.

As an aside: I would like ":results graphics" to partially revert its
old behavior before Org 9.3.  Prior to then, ob-R could generate a plot
with

:results graphics :file filename.png

but since commit 26ed66b23, we require the more verbose

:results graphics file :file filename.png

which seems unnecessarily verbose (since ":results graphics" doesn't do
anything without ":results file"), and also annoyingly broke many of my
Org documents before 2020.  I think it would be better if ":results
graphics" was equivalent to ":results graphics file", and may propose a
patch for this in future.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-doc-org-manual-Clarify-undocumented-uses-of-results-.patch --]
[-- Type: text/x-patch, Size: 2847 bytes --]

From 538c464ca88c8a1646a1b80352f0c8fb9f114f08 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Wed, 5 Jul 2023 19:02:25 -0700
Subject: [PATCH] doc/org-manual: Clarify undocumented uses of :results file

(Type):
(Format): Document that :results file is using the source block result
as file path when :file header argument is not present.  Document that
some Babel languages require :results graphics for plotting.
---
 doc/org-manual.org | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index 71ad4d9e8..3bf1c7d7f 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -18558,11 +18558,7 @@ described in the documentation for individual languages.  See
   #+cindex: @samp{file-ext}, header argument
   If =file= header argument is missing, Org generates the base name of
   the output file from the name of the code block, and its extension
-  from the =file-ext= header argument.  In that case, both the name
-  and the extension are mandatory.
-
-  Result can also be interpreted as path to file.  See =:results
-  link=.
+  from the =file-ext= header argument.
 
   #+begin_example
   ,#+name: circle
@@ -18572,6 +18568,30 @@ described in the documentation for individual languages.  See
   ,#+END_SRC
   #+end_example
 
+  If both =file= and =file-ext= header arguments are missing, then
+  result is interpreted as path to file.
+
+  #+begin_example
+  ,#+BEGIN_SRC python :results file
+  import matplotlib.pyplot as plt
+  import numpy as np
+
+  fname = "path/to/file.png"
+
+  plt.plot(np.arange(5))
+  plt.savefig(fname)
+
+  return fname # return filename to org-mode
+  ,#+END_SRC
+  #+end_example
+
+  When =file= or =file-ext= header arguments are present, you can
+  prevent Org from directly writing to that file by using =:results
+  link= or =:results graphics=.  This might be desirable if you write
+  to the file within the code block itself.  Some Babel languages also
+  require these extra header arguments for plotting.  See =:results
+  link= for more details.
+
   #+cindex: @samp{file-desc}, header argument
   The =file-desc= header argument defines the description (see [[*Link
   Format]]) for the link.  If =file-desc= is present but has no value,
@@ -18644,8 +18664,11 @@ follows from the type specified above.
   [[file:org-mode-unicorn.svg]]
   #+end_example
 
-  If =:file= header argument is omitted, interpret source block result
-  as the file path.
+  =:results file graphics= is also needed by some languages for
+  plotting, such as ob-R, ob-julia, and ob-octave, because they save
+  plots to file via wrapper code in their respective languages rather
+  than via elisp.  Consult the documentation for the respective Babel
+  languages for more details.
 
 - =org= ::
 
-- 
2.41.0

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

Jack Kamm <jackkamm@gmail.com> writes:

> In fact, :results file also works this way for other Babel languages.
> And I used this behavior before for plotting with ob-reticulate blocks.
>
> I attach a patch to fix the documentation in the manual about this.

Your patch appear to only add more confusion, IMHO.

I feel that the description about :results file is confusing from the
very beginning:

   ‘file’
        Interpret as a filename.  Save the results of execution of the code
        block to that file, then insert a link to it.  You can control both
        the filename and the description associated to the link.

:results file may currently imply three things:

1. Results of evaluation are the _contents_ of a file
2. Results of evaluation are the path to a file
3. Results of evaluation are discarded and Org just inserts a constant
   link, derived from header arguments.

(3) is used for :results file graphics/:results file link
(2) is used when Org is unable to deduce the file name from
    :file/:file-ext+#+name
(1) is used when the file name can be deduced from src block params    

> As an aside: I would like ":results graphics" to partially revert its
> old behavior before Org 9.3.  Prior to then, ob-R could generate a plot
> with
>
> :results graphics :file filename.png
>
> but since commit 26ed66b23, we require the more verbose
>
> :results graphics file :file filename.png
>
> which seems unnecessarily verbose (since ":results graphics" doesn't do
> anything without ":results file"), and also annoyingly broke many of my
> Org documents before 2020.  I think it would be better if ":results
> graphics" was equivalent to ":results graphics file", and may propose a
> patch for this in future.

Sounds reasonable.

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> Your patch appear to only add more confusion, IMHO.
>
> I feel that the description about :results file is confusing from the
> very beginning:

Well, I guess ":results file" has confusing behavior. So it's
difficult to write accurate, comprehensive, non-confusing
documentation for it ;)

> :results file may currently imply three things:
>
> 1. Results of evaluation are the _contents_ of a file
> 2. Results of evaluation are the path to a file
> 3. Results of evaluation are discarded and Org just inserts a constant
>    link, derived from header arguments.
>
> (3) is used for :results file graphics/:results file link
> (2) is used when Org is unable to deduce the file name from
>     :file/:file-ext+#+name
> (1) is used when the file name can be deduced from src block params    

Laying out the 3 behaviors this way seems clearer.

But I disagree that ":results graphics" means (3). It can behave as
(1) or (3), depending on the language. 

In practice (1) is the more common usage by far [*], and is also the
original intended use case [**].

(3) is just what happens when a Babel language doesn't implement
graphics (because then Org doesn't know how to save the graphical
results [***]).

If a Babel language doesn't implement graphics handling, the user can
workaround it by manually saving the plot. But this is just a
workaround, and we should discourage doing this with ":results
graphics", because it causes problems later if the language wants to
implement graphics support. Instead, ":results link" is the correct
usage for this case -- it always behaves as (3).

It is a mistake for the Org manual to define ":results link" and
":results graphics" as equivalent, because it is counter to the common
usage and understanding of ":results graphics", and because functions
like `org-babel-graphical-output-file' behave differently with
":results graphics". And what is the benefit of having 2 header
arguments with the same meaning? It is much more useful to define
":results graphics" for returning graphical output -- and indeed, that
is how ":results graphics" is generally used in practice.

[*] For example, nearly every code block in Worg with ":results
graphics" behaves as (1). Checking "grep results.\*graphics", I found
that 15 out of 16 code blocks behaved this way, inserting the
graphical results to file. The lone exception was in
ob-doc-clojure.org.

[**] ":results graphics" was originally created to implement (1) for
ob-R graphical results. See org commit 6bcbdce94, worg commit
8c49402c, and Version 7.5 Release Notes in worg/org-release-notes.org.
Also, see the Org Babel paper, which uses ":results graphics" with
behavior (1), not (3):

https://www.jstatsoft.org/article/view/v046i03

[***] Specifically, `org-babel-execute-src-block' doesn't know
anything about graphics, and needs to delegate to language-specific
implementation for that.

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

Jack Kamm <jackkamm@gmail.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Your patch appear to only add more confusion, IMHO.
>>
>> I feel that the description about :results file is confusing from the
>> very beginning:
>
> Well, I guess ":results file" has confusing behavior. So it's
> difficult to write accurate, comprehensive, non-confusing
> documentation for it ;)

Not necessarily confusing. We just need to clearly separate :results
file, :results graphics file, and :results file link.

":results file" imply that results of the code block are written to a file
(the file is specified using header args).

":results file link" imply that results of the code block are interpreted
as file link. The fact that presence of :file header arg overrides this
behaviour is something we may want to reconsider - it is confusing.

":results graphics file" imply that graphics generated during code block
execution is saved to file specified in the :file header args.
This feature is only available for some backends that can derive
graphics data from the source block. When :file is not specified, using
the actual code block output is confusing, and we may want to reconsider
this behaviour.

>> :results file may currently imply three things:
>>
>> 1. Results of evaluation are the _contents_ of a file
>> 2. Results of evaluation are the path to a file
>> 3. Results of evaluation are discarded and Org just inserts a constant
>>    link, derived from header arguments.
> ...
> Laying out the 3 behaviors this way seems clearer.
>
> But I disagree that ":results graphics" means (3). It can behave as
> (1) or (3), depending on the language. 
>
> In practice (1) is the more common usage by far [*], and is also the
> original intended use case [**].

Sorry, but I do not fully understand.
Generated graphics is not what Org sees as "results of evaluation".
I think it is well illustrated by

  #+begin_src R :file img.png
  hist(rnorm(100))
  "img.png is going to contain this string."
  #+end_src

  #+begin_src R :file img.png :results graphics
  hist(rnorm(100))
  "But now img.png is going to contain graphics."
  #+end_src

The latter has nothing to do with block output, which is a string.

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> ":results file" imply that results of the code block are written to a file
> (the file is specified using header args).
>
> ":results file link" imply that results of the code block are interpreted
> as file link. The fact that presence of :file header arg overrides this
> behaviour is something we may want to reconsider - it is confusing.

I think this is a lot clearer and more intuitive than the current
behavior, and worth doing.

But it is a breaking change, so it would be good to provide a variable
to re-enable the previous behavior, for back-compatibility of older
Org documents.

In particular, the Worg matplotlib example of ":results file" without
":file" header arg is fairly old, and I have a few Org docs using
":results file" this way as well. So I would appreciate a
backwards-compatibility variable if we change this.

> ":results graphics file" imply that graphics generated during code
> block execution is saved to file specified in the :file header args.
> This feature is only available for some backends that can derive
> graphics data from the source block.  When :file is not specified,
> using the actual code block output is confusing, and we may want to
> reconsider this behaviour.

I agree.

On a related note, I think we should revert most of b088389c6 on
bugfix branch. That documentation causes more harm than good, and is
based on some confusion in [1] ("graphics" and "link" are _not_
equivalent in general).

> Sorry, but I do not fully understand.
> Generated graphics is not what Org sees as "results of evaluation".
> I think it is well illustrated by
>
>   #+begin_src R :file img.png
>   hist(rnorm(100))
>   "img.png is going to contain this string."
>   #+end_src
>
>   #+begin_src R :file img.png :results graphics
>   hist(rnorm(100))
>   "But now img.png is going to contain graphics."
>   #+end_src
>
> The latter has nothing to do with block output, which is a string.

IMO block _value_ is string, while block _output_ is string AND
graphics.

So by my interpretation, ob-R is slightly incorrect in how it handles
":results output graphics" vs ":results value graphics".  IMO the more
technically correct approach is in the ob-python patch that I proposed
a couple years ago [2], and plan to revisit soon. In that patch,
ob-python ":results graphics output" will plot from pyplot.gcf(),
while ":results graphics value" will expect a matplotlib.Figure object
to be returned for plotting.

(Note I do _not_ suggest changing ob-R -- even if my interpretation is
correct, I think that common usage and backwards-compatibility
outweighs strict technical correctness in this case).

[1] https://list.orgmode.org/87fu41zcn2.fsf@nicolasgoaziou.fr/
[2] https://list.orgmode.org/87eenpfe77.fsf@gmail.com/

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

Jack Kamm <jackkamm@gmail.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> ":results file" imply that results of the code block are written to a file
>> (the file is specified using header args).
>>
>> ":results file link" imply that results of the code block are interpreted
>> as file link. The fact that presence of :file header arg overrides this
>> behaviour is something we may want to reconsider - it is confusing.
>
> I think this is a lot clearer and more intuitive than the current
> behavior, and worth doing.
>
> But it is a breaking change, so it would be good to provide a variable
> to re-enable the previous behavior, for back-compatibility of older
> Org documents.

Not necessarily. We may instead arrange org-lint and possibly ob-core to
throw a warning when an src block uses confusing setting combinations.

Without changing the underlying behaviour.

Basically, discourage using confusing staff.

> In particular, the Worg matplotlib example of ":results file" without
> ":file" header arg is fairly old, and I have a few Org docs using
> ":results file" this way as well. So I would appreciate a
> backwards-compatibility variable if we change this.

We should update the docs to avoid such examples.

> On a related note, I think we should revert most of b088389c6 on
> bugfix branch. That documentation causes more harm than good, and is
> based on some confusion in [1] ("graphics" and "link" are _not_
> equivalent in general).

We should generally rewrite that part of the manual, I think.
My previous message was a tentative outline on how the things should be
presented in the manual.

And I do not think that we should do it on bugfix. It is a non-trivial
change, so it should go on main.

>> Sorry, but I do not fully understand.
>> Generated graphics is not what Org sees as "results of evaluation".
>> I think it is well illustrated by
>>
>>   #+begin_src R :file img.png
>>   hist(rnorm(100))
>>   "img.png is going to contain this string."
>>   #+end_src
>>
>>   #+begin_src R :file img.png :results graphics
>>   hist(rnorm(100))
>>   "But now img.png is going to contain graphics."
>>   #+end_src
>>
>> The latter has nothing to do with block output, which is a string.
>
> IMO block _value_ is string, while block _output_ is string AND
> graphics.

From the point of view of ob-core.el, the output is stdout.
We even display stderr separately (except sessions, where they are
indistinguishable).

But I can see where the confusion is coming from.
"Output" can mean many things, including stdout, stderr, graphical
display (plot window), a file, a sound, or maybe even interactive
terminal ncurses interface.
I do not have a solid idea how to deal with such confusion on design
level.

> So by my interpretation, ob-R is slightly incorrect in how it handles
> ":results output graphics" vs ":results value graphics".  IMO the more
> technically correct approach is in the ob-python patch that I proposed
> a couple years ago [2], and plan to revisit soon. In that patch,
> ob-python ":results graphics output" will plot from pyplot.gcf(),
> while ":results graphics value" will expect a matplotlib.Figure object
> to be returned for plotting.

Sounds reasonable. Let me know if you need any help along the way.

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> We may instead arrange org-lint and possibly ob-core to throw a
> warning when an src block uses confusing setting combinations.
> Without changing the underlying behaviour.
> Basically, discourage using confusing staff.
> ...
> We should update the docs to avoid such examples.
> ...
> We should generally rewrite that part of the manual, I think.
> My previous message was a tentative outline on how the things should be
> presented in the manual.
> ...
>> IMO the more technically correct approach is in the ob-python patch
>> that I proposed a couple years ago [2], and plan to revisit soon. In
>> that patch, ob-python ":results graphics output" will plot from
>> pyplot.gcf(), while ":results graphics value" will expect a
>> matplotlib.Figure object to be returned for plotting.
>
> Sounds reasonable. Let me know if you need any help along the way.

Thank you. And likewise, I agree with your suggestions to update
org-lint, org-manual, and Worg, and will try to help where I can. (I
might be a little slow to start due to other deadlines the next couple
weeks).

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Liu Hui]

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

Ihor Radchenko <yantar92@posteo.net> 于2023年7月4日周二 19:29写道：

> That said, your patch should still work fine even with these
> considerations. But we may need to sort out this undocumented behaviour.
> Probably, an error like in `org-babel-graphical-output-file' should be
> thrown by `org-babel-generate-file-param'.

Yes. The main concern is that some users may rely on the undocumented
behavior.


> What about
>
> #+begin_src python :file "test.png" :results graphics file
> import matplotlib.pyplot as plt
> plt.plot([1,2,3,4,5])
> plt.savefig('test.png')
> plt.plot([5,4,3,2,1])
> plt.show()
> #+end_src

OK, I think that it is better to turn off the feature by default and I
have updated the patch. Inspired by ob-R, some extra header arguments
are supported. Thanks.

[-- Attachment #2: 0001-ob-python-Graphics-output-enhancement.patch --]
[-- Type: text/x-patch, Size: 3367 bytes --]

From 5f3fc8602c0eb1f6d0c072bd0c1a6bb883b8c9f9 Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Wed, 5 Jul 2023 12:48:36 +0800
Subject: [PATCH] ob-python: Graphics output enhancement

* lisp/ob-python.el (org-babel-python-graphics-output-function): New
variable.
(org-babel-execute:python): Allow to save graphics output with a
user-defined function when the result type is `file graphics'.
(org-babel-python-matplotlib-graphics-output): New helper function to
generate code to save matplotlib-based graphics.
---
 lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 0e0539d7a..4c304f83b 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -62,6 +62,14 @@ (defcustom org-babel-python-None-to 'hline
   :package-version '(Org . "8.0")
   :type 'symbol)
 
+(defvar org-babel-python-graphics-output-function nil
+  "A function generating Python code for producing graphics output.
+Specifically, for src blocks with `:results file graphics' header
+argument, the code returned by the function will be appended to
+the end of the src block and should produce the graphics file.
+It can be set to `org-babel-python-matplotlib-graphics-output'
+for matplotlib-based graphics.")
+
 (defun org-babel-execute:python (body params)
   "Execute a block of Python code with Babel.
 This function is called by `org-babel-execute-src-block'."
@@ -72,6 +80,9 @@ (defun org-babel-execute:python (body params)
 		   (cdr (assq :session params))))
          (result-params (cdr (assq :result-params params)))
          (result-type (cdr (assq :result-type params)))
+         (graphics-file (and (member "graphics" result-params)
+                             (ignore-errors
+                               (org-babel-graphical-output-file params))))
 	 (return-val (when (eq result-type 'value)
 		       (cdr (assq :return params))))
 	 (preamble (cdr (assq :preamble params)))
@@ -81,6 +92,10 @@ (defun org-babel-execute:python (body params)
 	   (org-babel-expand-body:generic
 	    body params
 	    (org-babel-variable-assignments:python params))
+           (when graphics-file
+             (if (functionp org-babel-python-graphics-output-function)
+                 (funcall org-babel-python-graphics-output-function
+                          graphics-file params)))
 	   (when return-val
 	     (format (if session "\n%s" "\nreturn %s") return-val))))
          (result (org-babel-python-evaluate
@@ -149,6 +164,24 @@ (defun org-babel-python-table-or-string (results)
                 res)
       res)))
 
+(defun org-babel-python-matplotlib-graphics-output (out-file params)
+  "Return the Python code for saving graphics to `OUT-FILE'."
+  (let* ((allowed-args '(:dpi :format :backend))
+         (args (mapconcat
+	        (lambda (pair)
+		  (if (member (car pair) allowed-args)
+		      (format ",%s=%S"
+			      (substring (symbol-name (car pair)) 1)
+			      (cdr pair)) ""))
+	        params "")))
+    (format "
+import matplotlib.pyplot
+if len(matplotlib.pyplot.get_fignums()) > 0:
+    matplotlib.pyplot.gcf().savefig('%s'%s)
+else:
+    raise RuntimeWarning('No figure is found')"
+            out-file args)))
+
 (defvar org-babel-python-buffers '((:default . "*Python*")))
 
 (defun org-babel-python-session-buffer (session)
-- 
2.25.1

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> Liu Hui <liuhui1610@gmail.com> writes:
>
>> This patch adds graphics output support for ob-python via matplotlib.
>> Specifically, it allows to use header argument `:results file
>> graphics' as follows:
>>
>> #+begin_src python :file "test.png" :results graphics file
>> import matplotlib.pyplot as plt
>> plt.plot([1,2,3,4,5])
>> #+end_src
>
> This might be a useful feature, but it will break the existing
> conventions, if used as in the patch. Currently, the above combination
> of :file and :results is treated as the following:

":results graphics file" is used this way in ob-R and ob-julia, and also
in the testing files test-ob-octave.el and ob-maxima-test.org.

":results graphics" just means that the result from
org-babel-execute:lang isn't written to the file by
org-babel-execute-src-block. This is needed for plotting because
org-babel-execute:lang usually writes directly to the file, rather than
returning a byte stream for the PNG or SVG.

So the Org manual's wording about
"side-effects" and "output not written to disk" is correct in a sense,
but also confusing (readers should not have to know about these internal
implementation details).

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Ihor Radchenko]

Jack Kamm <jackkamm@gmail.com> writes:

> ":results graphics file" is used this way in ob-R and ob-julia, and also
> in the testing files test-ob-octave.el and ob-maxima-test.org.
>
> ":results graphics" just means that the result from
> org-babel-execute:lang isn't written to the file by
> org-babel-execute-src-block. This is needed for plotting because
> org-babel-execute:lang usually writes directly to the file, rather than
> returning a byte stream for the PNG or SVG.
>
> So the Org manual's wording about
> "side-effects" and "output not written to disk" is correct in a sense,
> but also confusing (readers should not have to know about these internal
> implementation details).

What you describe is a special feature in _some_ babel backends. More
generally, users must take care about writing the file to disk by
themselves, as illustrated in the wget example.

It may be worth mentioning though that some babel backends take care
about writing the file.

-- 
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>

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Liu Hui <liuhui1610@gmail.com> writes:

> Hi,
>
> This patch adds graphics output support for ob-python via matplotlib.
> Specifically, it allows to use header argument `:results file
> graphics' as follows:
>
> #+begin_src python :file "test.png" :results graphics file
> import matplotlib.pyplot as plt
> plt.plot([1,2,3,4,5])
> #+end_src
>
> The feature is described in the documentation as follows and has been
> supported by ob-R, ob-julia, etc.

Thanks -- I haven't had a chance to test drive this yet, but I do have a
couple questions:

1. Do you need to add a call to pyplot.gcf().clear(), in case of
multiple blocks in a session?

2. Would it make sense to wrap in pyplot.rc_context, so that we can use
the :width and :height arguments like ob-R? E.g.,

with pyplot.rc_context({'figure.figsize': (8,5)}):
     pyplot.plot([1,2,3,4,5])
     pyplot.gcf().savefig('filename.png')

Will create a png file with width 8 and height 5.

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Liu Hui]

Jack Kamm <jackkamm@gmail.com> 于2023年7月5日周三 13:13写道：

> 1. Do you need to add a call to pyplot.gcf().clear(), in case of
> multiple blocks in a session?

I don't think so. Some users may want to keep the figure between
blocks, and they can always clear the figure themselves when
necessary.

> 2. Would it make sense to wrap in pyplot.rc_context, so that we can use
> the :width and :height arguments like ob-R? E.g.,
>
> with pyplot.rc_context({'figure.figsize': (8,5)}):
>      pyplot.plot([1,2,3,4,5])
>      pyplot.gcf().savefig('filename.png')
>
> Will create a png file with width 8 and height 5.

Thanks for your suggestion and I have added some arguments (e.g. :dpi)
in the updated patch. But rc_context doesn't work reliably with
multiple blocks in a session, i.e., the figure size may not change.

BTW, I have updated the patch to turn off the feature by default,
since it may break existing src blocks using `graphics'. WDYT? Thanks.

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Liu Hui <liuhui1610@gmail.com> writes:

> I don't think so. Some users may want to keep the figure between
> blocks, and they can always clear the figure themselves when
> necessary.

I'd rather not have to call pyplot.gcf().clear() every time, it doesn't
seem nice.  ob-R doesn't require manually clearing the plot, and neither
do Jupyter notebooks.

I would propose the following instead: for ":results output graphics",
ob-python should plot the gcf, and clear it beforehand. But for
":results value graphics", the ob-python block should return a
matplotlib Figure object to plot, which would allow keeping and
modifying a Figure between blocks.

I actually proposed that behavior before in this patch:

https://list.orgmode.org/87eenpfe77.fsf@gmail.com/

But never wound up applying it -- the patch was rather large, with a lot
of extra features, and I wasn't sure they were all worth the extra
complexity.  Then life got in the way, and I never got around to
revisiting ob-python plotting, until now.

> BTW, I have updated the patch to turn off the feature by default,
> since it may break existing src blocks using `graphics'. WDYT? Thanks.

I don't think the defcustom is necessary at this point. The feature
makes ob-python consistent with similar Babel languages like ob-R, and I
can't think of any existing use case of ob-python with ":results
graphics" that this would break. In case I missed some obscure edge
case, we can ask forgiveness and redirect the user to ":results link"
instead.

The defcustom might be useful in the future in case we want to support a
non-matplotlib plotting system (such as ete3), but that is relatively
rare and I think it's better to keep it simple for now.

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Liu Hui]

Jack Kamm <jackkamm@gmail.com> 于2023年7月6日周四 11:49写道：

> I would propose the following instead: for ":results output graphics",
> ob-python should plot the gcf, and clear it beforehand. But for
> ":results value graphics", the ob-python block should return a
> matplotlib Figure object to plot, which would allow keeping and
> modifying a Figure between blocks.
>
> I actually proposed that behavior before in this patch:
>
> https://list.orgmode.org/87eenpfe77.fsf@gmail.com/
>
> But never wound up applying it -- the patch was rather large, with a lot
> of extra features, and I wasn't sure they were all worth the extra
> complexity.  Then life got in the way, and I never got around to
> revisiting ob-python plotting, until now.

I think your proposal about ":results graphics" is more flexible and
complies the documentation. Since the patch has no real problem and
the feature is useful indeed, I hope it can be merged instead of mine
after the problem of documentation is resolved.

As for other features in the patch, maybe it is better to convert
dict/dataframe/array to table/list only when the result type is
explicitly set to table or list?

Re: [PATCH] ob-python: support header argument `:results file graphics'

[Jack Kamm]

Liu Hui <liuhui1610@gmail.com> writes:

> I think your proposal about ":results graphics" is more flexible and
> complies the documentation. Since the patch has no real problem and
> the feature is useful indeed, I hope it can be merged instead of mine
> after the problem of documentation is resolved.

Thanks for your gracious words.  I will aim to revisit and finish this
work in a month or so -- I'm swamped the next couple weeks, and
anyways we first need to sort out the issues around documentation,
back-compatibility, and general semantics of ":results graphics".

> As for other features in the patch, maybe it is better to convert
> dict/dataframe/array to table/list only when the result type is
> explicitly set to table or list?

Thanks, that is an interesting idea. Especially for dict, which can be
interpreted in many ways, it is a good idea to make use of the
table/list/etc type specifications.