Re: `with` as a list.

[Mario Frasca]

Hi Kyle,

thank you for writing back, I have a couple of questions in reply.

btw. are you on irc.freenode.net?  I'm `mariotomo' there.  and I just 
joined `#org-mode'.  I don't think that my terminal will ring a bell if 
I'm mentioned there.

On 30/05/2020 01:22, Kyle Meyer wrote:
> Mario Frasca writes:
>
> > […]
> 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.)

thank you for taking the time to review!


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

I'm editing in my cloned repository, and committing often, so I do not 
have a single commit, consequently also not a single commit message.

I just sent my form request to assign@gnu.org.


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

ok, 1 and 2 are just consequence of my usual way of typing, however 
wrong, I will fix them.  3 I didn't consider.  useful point.


> > +  ;; make 'deps explicit
> I think this and the other comments in this function could safely be
> dropped.

:+1:


> > +  (unless deps
> > +    (setf deps (let (r)
> > +                (dotimes (i num-cols r)
> > +                  (unless (eq num-cols (+ ind i))
> > +                    (setq r (cons (- num-cols i) r)))))))
> […] using setq unless setf is needed would be more
> consistent with this code base.

the "unless needed" makes me suspect I should read what's the difference.


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

no, that would break functionality: I need to keep the given order.

btw my patch allows you to use the same column more than once.


> > +  ;; 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`.

it is needed: I'm not extending any `with' given as a non-empty list.

but I should be using some settings, some global default `with' value, 
which I don't know where to find.  hard coding "lines" can't be the 
right thing to do.

> > +         (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.

I'm not sure about the TCO (in other words: I haven't the faintest idea 
what you mean by that), and I would not know how to write this using 
`cl-loop'.  I'll have a look though.


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

the whole org-plot.el has (had) no unit tests, so if you share a couple 
of your org-plot invocations, I can convert them to regression tests.


> > @@ -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'.

yes, definitely!  as said, I'm not a lisp programmer.  ;-)

I hope to post a new diff soon.

cheers, MF