[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: `with` as a list.
From: |
Mario Frasca |
Subject: |
Re: `with` as a list. |
Date: |
Sat, 30 May 2020 09:23:21 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 |
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
- `with` as a list., Mario Frasca, 2020/05/22
- [PATCH] [FEATURE] Re: `with` as a list., Mario Frasca, 2020/05/28
- Re: `with` as a list., Kyle Meyer, 2020/05/30
- Re: `with` as a list.,
Mario Frasca <=
- Re: `with` as a list., Mario Frasca, 2020/05/30
- Re: `with` as a list., Kyle Meyer, 2020/05/30
- Re: `with` as a list., Mario Frasca, 2020/05/30
- Re: `with` as a list., Mario Frasca, 2020/05/31
- Re: `with` as a list., Kyle Meyer, 2020/05/31
- Re: `with` as a list., Mario Frasca, 2020/05/31
- Re: `with` as a list., Kyle Meyer, 2020/05/31
Re: `with` as a list., Mario Frasca, 2020/05/30