Re: `with` as a list.

[Kyle Meyer <kyle@kyleam.com>]

Mario Frasca writes:

> now and then I use emacs to make graphs.  now recently I was plotting 
> point data, and a running average "fit", so I wanted to have points, and 
> lines, which I know it's possible in `gnuplot` but now how do I do that 
> from org-plot …
>
> I wrote a small patch for org-plot.el, I'm not a Lisp programmer so I'm 
> sure the patch looks terrible, but it does allow me to do this:
>
> #+PLOT: ind:1 deps:(3 6 4 7) with:(points lines points lines)
>
> it's two additions: [...]

Thanks for the patch and for clearly describing the motivation.  I'm not
an org-plot user, but the change sounds useful.  (It'd be great if
org-plot users could chime in.)

Two meta-comments:

  * Please provide a patch with a commit message.  See
    <https://orgmode.org/worg/org-contribute.html> for more information.

  * The link above also explains the copyright assignment requirement
    for contributions.  Please consider assigning copyright to the FSF.

> diff --git a/lisp/org-plot.el b/lisp/org-plot.el
> index a23195d2a..87a415137 100644
> --- a/lisp/org-plot.el
> +++ b/lisp/org-plot.el
> @@ -179,6 +179,28 @@ and dependent variables."
>  	  (setf back-edge "") (setf front-edge ""))))
>      row-vals))
>  
> +(defun org-plot/zip-deps-with (num-cols ind deps with)
> +  "describe each column to be plotted as (col . with)"

This doesn't match the convention used in this code base for docstrings.
Taking a look around should give you a good sense, but (1) the first
word should be capitalized, (2) sentences should end with a period, and
(3) ideally all parameters should be described in the docstring.

> +  ;; make 'deps explicit

I think this and the other comments in this function could safely be
dropped.

> +  (unless deps
> +    (setf deps (let (r)
> +		 (dotimes (i num-cols r)
> +		   (unless (eq num-cols (+ ind i))
> +		     (setq r (cons (- num-cols i) r)))))))

Hmm, org-plot does seem to use setf a lot (more than any other .el file
in the repo), though using setq unless setf is needed would be more
consistent with this code base.

The code above looks fine to me.  Another option would be to use
number-sequence and then filter out the ind value.

> +  ;; make sure 'with matches 'deps
> +  (unless with
> +    (setf with "lines"))
> +  (unless (listp with)
> +    (setf with (mapcar (lambda (x) with) deps)))

This is make-list in the more recent diff you sent, which I agree is
better.

> +  ;; invoke zipping function on converted data
> +  (org-plot/zip deps with))
> +
> +(defun org-plot/zip (xs ys)
> +  (unless
> +      (null xs)
> +    (cons (cons (car xs) (or (car ys) "lines"))

Is the "lines" fall back ever reached?  It looks like you're extending
`with' above to be the same length as `deps`.

> +	  (org-plot/zip (cdr xs) (cdr ys)))))

In Elisp, it's not very common to use recursive functions (and there's
no TCO).  In the case of zipping two lists, I think it'd probably be
easiest to just use cl-loop.  And in my view having a separate function
here is probably an overkill.

>  (defun org-plot/gnuplot-script (data-file num-cols params &optional preface)
>    "Write a gnuplot script to DATA-FILE respecting the options set in PARAMS.
>  NUM-COLS controls the number of columns plotted in a 2-d plot.
> @@ -240,22 +262,22 @@ manner suitable for prepending to a user-specified script."
>  			       "%Y-%m-%d-%H:%M:%S") "\"")))
>      (unless preface
>        (pcase type			; plot command
> -	(`2d (dotimes (col num-cols)
> -	       (unless (and (eq type '2d)
> -			    (or (and ind (equal (1+ col) ind))
> -				(and deps (not (member (1+ col) deps)))))
> -		 (setf plot-lines
> -		       (cons
> -			(format plot-str data-file
> -				(or (and ind (> ind 0)
> -					 (not text-ind)
> -					 (format "%d:" ind)) "")
> -				(1+ col)
> -				(if text-ind (format ":xticlabel(%d)" ind) "")
> -				with
> -				(or (nth col col-labels)
> -				    (format "%d" (1+ col))))
> -			plot-lines)))))
> +	(`2d (dolist
> +		 (col-with
> +		  (org-plot/zip-deps-with num-cols ind deps with))
> +	       (setf plot-lines
> +		     (cons
> +		      (format plot-str data-file
> +			      (or (and ind (> ind 0)
> +				       (not text-ind)
> +				       (format "%d:" ind)) "")
> +			      (car col-with)
> +			      (if text-ind (format ":xticlabel(%d)" ind) "")
> +			      (cdr col-with)
> +			      (or (nth (1- (car col-with))
> +				       col-labels)
> +				  (format "%d" (car col-with))))
> +		      plot-lines))))

I haven't looked at this bit too closely (and I know the handling around
col-labels changed a bit in the last message you sent), but I did try
out a few org-plot invocations and things seemed to work as I expected.
I'll plan to take a closer when you send a reroll.

> @@ -320,7 +343,7 @@ line directly before or after the table."
>  		 0)
>  	      (plist-put params :timeind t)
>  	    ;; Check for text ind column.
> -	    (if (or (string= (plist-get params :with) "hist")
> +	    (if (or (and (stringp with) (string= with "hist"))

Could be simplified by using `equal'.

Thanks again for the patch.