Re: `with` as a list.

[Mario Frasca <mario@anche.no>]

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