help-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: simple first emacs script


From: Pascal J. Bourguignon
Subject: Re: simple first emacs script
Date: Wed, 15 Dec 2010 15:51:07 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux)

Tom <tom@somewhere.com> writes:

> I am about 2/3 way through the introduction to emacs lisp, and thought
> it would help my learning if I wrote tried writing a simple script to
> do something useful to me with the lessons I have covered so far so
> far.  I should point out I am surgeon not a computer scientist and
> know nothing more about programming than what is contained in the
> introduction to emacs lisp.
> I end up with this:
>
> (defun nirs-data-clean (number-of-channels)
> "Cleans the output files from the INVOS monitor removing all columns
> apart from StO2 values, and the date and time stamps. Sto2 columns to
> be retained are specified by the NUMBER-OF-CHANNELS variable.  It
> assumes that channels are always used in numerical order, i.e. channel
> 1 always, then 2, then 3 then 4."
> (interactive "nEnter number of channels (1-4): ")
[...]

First, unless the indenting was destroyed by your newsreader program,
you might want to make use of the indent-region command ( type C-M-\ )  
so that your source will look like:


(defun nirs-data-clean (number-of-channels)
  "Cleans the output files from the INVOS monitor removing all columns
apart from StO2 values, and the date and time stamps. Sto2 columns to
be retained are specified by the NUMBER-OF-CHANNELS variable.  It
assumes that channels are always used in numerical order, i.e. channel
1 always, then 2, then 3 then 4."
  (interactive "nEnter number of channels (1-4): ")
  (barf-if-buffer-read-only)
  (require 'csv-mode)
  (push-mark (point-max))
  (goto-char (point-min))
  (if (or (eq number-of-channels 1)
          (eq number-of-channels 2)
          (eq number-of-channels 3)
          (eq number-of-channels 4))
      (or (when (eq number-of-channels 1)
            (csv-kill-fields '(4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
                               21 22 23 24 25 26 27 28 29 30 31 32 33 34) 
(point) (mark)))
          (when (eq number-of-channels 2)
            (csv-kill-fields '(4 5 6 7 8 9 11 12 13 14 15 16 17 18 19 20 21
                               22 23 24 25 26 27 28 29 30 31 32 33 34) (point) 
(mark)))
          (when (eq number-of-channels 3)
            (csv-kill-fields '(4 5 6 7 8 9 11 12 13 14 15 16 18 19 20 21 22
                               23 24 25 26 27 28 29 30 31 32 33 34) (point) 
(mark)))
          (when (eq number-of-channels 4)
            (csv-kill-fields '(4 5 6 7 8 9 11 12 13 14 15 16 18 19 20 21 22
                               23 25 26 27 28 29 30 31 32 33 34) (point) (mark))
            (message "%s is not a valid number of channels.  Please enter a
number between 1 and 4." number-of-channels))))
  (when (y-or-n-p "Replace 0 StO$_2$ values with na: ")
    (progn (goto-char (point-min))
           (while (re-search-forward "[^[:digit:]][0][^[:digit:]]" nil t)
             (replace-match "na ")))))

Instead of calling barf-if-buffer-read-only, you can just prefix the
interactive string with a star:

    (interactive "*nEnter number of channels (1-4): ")

The (require 'csv-mode) form would be better placed on the toplevel
(ie. above the defun form).


Instead of push-mark (I don't see the matching pop-mark), you might use
the save-excursion macro.

Instead of using eq, you might prefer to use eql by default.  In the
case of numbers, it would be better to use =.

Instead of:
    
    (or (when ... ...)
        (when ... ...)
        ...)

you can write:

    (cond (... ...)
          (... ...)
          ...)

And since all your conditions are about the same variable, matching a
set of value comparable with eql, you could instead use case:

    (case number-of-channels
      ((1)  ...)
      ((2)  ...)
      ((3)  ...)
      ((4)  ...)
      (otherwise (error ...)))

Notice how the indentation shows that your message form is not placed at
the right level.  You intended it to be the else branch of the if form,
but you put it inside the last when body.  Also you probably want to use
error here instead of message, since if the number of channel is wrong,
you probably don't want to replace the StO2 zeroes.


    You may want to use paredit, which helps editing lisp code in a
    structural way.

    http://www.emacswiki.org/ParEdit
    http://www.emacswiki.org/emacs/PareditCheatsheet
    http://mumble.net/~campbell/emacs/paredit.el

    Just download this paredit.el file,  and put it in

      /usr/share/emacs/site-lisp/

    (check that this path is in load-path, typing C-h v load-path RET.  If
    it is not there, then use the path of the site-lisp directory listed in
    load-path),

    then put:

       (require 'cl)
       (require 'paredit)
       (push 'paredit-mode emacs-lisp-mode-hook)
       (push 'paredit-mode       lisp-mode-hook)

    in your ~/.emacs file.

    If you want to activate immediately, without restarting emacs, you may
    reload ~/.emacs with M-x load-file RET ~/.emacs RET


Finally, since the bodies of each case branch are the same, you may
distribute it around and write:

    (csv-kill-fields (case number-of-channels
                             ((1) '(4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 
20 21 22 23 24 25 26 27 28 29 30 31 32 33 34))
                             ((2) '(4 5 6 7 8 9    11 12 13 14 15 16 17 18 19 
20 21 22 23 24 25 26 27 28 29 30 31 32 33 34))
                             ((3) '(4 5 6 7 8 9    11 12 13 14 15 16    18 19 
20 21 22 23 24 25 26 27 28 29 30 31 32 33 34))
                             ((4) '(4 5 6 7 8 9    11 12 13 14 15 16    18 19 
20 21 22 23    25 26 27 28 29 30 31 32 33 34))
                             (otherwise
                              (message "%s is not a valid number of channels.  
Please enter a number between 1 and 4."
                                       number-of-channels)))
                           start end)

But then, it might be clearer to give an intensional definition instead
of an extensional one.  A practical operator would be iota:

    (defun iota (count &optional start step)
      "
    RETURN:   A list containing the elements 
              (start start+step ... start+(count-1)*step)
              The start and step parameters default to 0 and 1, respectively. 
              This procedure takes its name from the APL primitive.
    EXAMPLES: (iota 5) => (0 1 2 3 4)
              (iota 5 0 -0.1) => (0 -0.1 -0.2 -0.3 -0.4)
    "
      (setf start (or start 0) step (or step 1))
      (when (< 0 count)
        (do ((result '())
             (item (+ start (* step (1- count))) (- item step)))
            ((< item start) result)
          (push item result))))


So you could write:

    (csv-kill-fields (set-difference (iota 34 1)
                                     (ecase number-of-channels
                                       ((1) '(1 2 3))
                                       ((2) '(1 2 3 10))
                                       ((3) '(1 2 3 10 17))
                                       ((4) '(1 2 3 10 17 24))))
                     (point-min) (point-max))

We may remove the otherwise branch, assuming we only get correct
number-of-channels (and using ecase instead of case to signal an error
if not), which we can ensure by using a more sophisticated interactive
form, using competing-read:

    (interactive
       (list
        (first (read-from-string
                (completing-read "Enter number of channels (1-4): "
                                 (mapcar (lambda (x) (cons (format "%d" x) 
nil)) (iota 4 1))
                                 (function identity)
                                 t                 ; require match
                                 "" nil "1" nil)))))
    (barf-if-buffer-read-only) ; in this case, we need to call it
                               ; separately. 


Be sure to read the documentation of each operator used, eg. with:
    C-h f interactive RET 
    C-h f completing-read RET 
etc.




when (and unless and other macros) has an implicit progn, so there's no
need to embed one in it.



So to wrap it up so far, you could have:


    (defun nirs-data-clean (number-of-channels)
      "Cleans the output files from the INVOS monitor removing all columns
    apart from StO2 values, and the date and time stamps. Sto2 columns to
    be retained are specified by the NUMBER-OF-CHANNELS variable.  It
    assumes that channels are always used in numerical order, i.e. channel
    1 always, then 2, then 3 then 4."
      (interactive
       (list
        (first (read-from-string
                (completing-read "Enter number of channels (1-4): "
                                 (mapcar (lambda (x) (cons (format "%d" x) 
nil)) (iota 4 1))
                                 (function identity)
                                 t                 ; require match
                                 "" nil "1" nil)))))
      (barf-if-buffer-read-only)
      (save-excursion
        (let ((start (point-min))
              (end   (point-max)))
          (goto-char start)
          (csv-kill-fields (set-difference (iota 34 1)
                                           (ecase number-of-channels
                                             ((1) '(1 2 3))
                                             ((2) '(1 2 3 10))
                                             ((3) '(1 2 3 10 17))
                                             ((4) '(1 2 3 10 17 24))))
                           start end)
          (when (y-or-n-p "Replace 0 StO$_2$ values with na: ")
            (goto-char start)
            (while (re-search-forward "[^[:digit:]][0][^[:digit:]]" nil t)
              (replace-match "na "))))))



> The idea is to automatically clean unwanted columns from csv (space
> separated) data file and ask me if I want to replace 0 values with
> na. 


Ah, if you read the documentation of csv-kill-fields,  you will see that
it depends on the right setting of the variable csv-separators to know
what separator to use.  By default I have it set to a comma.  So you
want to bind this variable in your function:

    (let ((csv-separators '(" ")))
      (csv-kill-fields ...))

Note however that it is a single character string, and that your fields
are separated by several.  When I try it, it fails with csv-kill-fields
complaining about the number of columns.  It is probably better to use
commas to separate the fields, so:

    (defun nirs-data-clean (number-of-channels)
      "Cleans the output files from the INVOS monitor removing all columns
    apart from StO2 values, and the date and time stamps. Sto2 columns to
    be retained are specified by the NUMBER-OF-CHANNELS variable.  It
    assumes that channels are always used in numerical order, i.e. channel
    1 always, then 2, then 3 then 4."
      (interactive
       (list
        (first (read-from-string
                (completing-read "Enter number of channels (1-4): "
                                 (mapcar (lambda (x) (cons (format "%d" x) 
nil)) (iota 4 1))
                                 (function identity)
                                 t                 ; require match
                                 "" nil "1" nil)))))
      (barf-if-buffer-read-only)
      (save-excursion
        (replace-regexp " +" "," nil (point-min) (point-max))
        (goto-char (point-min))
        (let ((csv-separators '(",")))
          (csv-kill-fields (set-difference (iota 34 1)
                                           (ecase number-of-channels
                                             ((1) '(1 2 3))
                                             ((2) '(1 2 3 10))
                                             ((3) '(1 2 3 10 17))
                                             ((4) '(1 2 3 10 17 24))))
                           (point-min) (point-max)))
        (when (y-or-n-p "Replace 0 StO$_2$ values with na: ")
          (goto-char (point-min))
          (while (re-search-forward "[^[:digit:]]0[^[:digit:]]" nil t)
            (replace-match "na ")))
        ;; let's put back spaces as separators:
        (replace-regexp "," " " nil (point-min) (point-max))))


> I appreciate this is very messy, and I should be able to specify
> ranges of field rather than listing every single one (but I couldn't
> get that to work), but it works kind of.  When I evaluate it and run
> it for the first time it works, but if I undo the changes to the file
> and run it again it often only takes out the 4th column, if I reopen
> the file or re-evaluate the script it usually works again but I can't
> seem to work the pattern of working and not working - as I can't see
> the pattern I can't learn where I am going wrong.

I see no reason why that would happen.  In any case, my version seems to
work correctly, even after undoing and running it again.

> I assume I have made an obvious mistake but I don't have the
> experience to know what it is.  Any general comments about
> simple/better ways to clean this script would be welcome, for example
> would I be better making the regexp replace a yes/no arg to the
> function so if I call this script from another script I can specify a
> y/n argument (I don't know how to do this anyway though).

Indeed, this is a good intension.  You can separate interactive user
interface commands from the functions doing the actual work, so that you
can reuse the later.  The interactive form allow you to merge both
usages.

You only have to add the wanted parameter, and specify it in the
interactive form.  You can also specify some parameters optional:


    (defun nirs-data-clean (number-of-channels &optional replace-StO2-0-p)
       ...)

The simple interactive form would use a multiline string:

      (interactive "*nEnter number of channels (1-4):
sReplace 0 StO$_2$ values with na (y/n): ")

Unfortunately, interactive doesn't provide for a 'boolean' input, so
we'll keep using the sophisticated form:

    (defun nirs-data-clean (number-of-channels &optional replace-StO2-0-p)
      "Cleans the output files from the INVOS monitor removing all columns
    apart from StO2 values, and the date and time stamps. Sto2 columns to
    be retained are specified by the NUMBER-OF-CHANNELS variable.  It
    assumes that channels are always used in numerical order, i.e. channel
    1 always, then 2, then 3 then 4."
      (interactive
       (list
        (first (read-from-string
                (completing-read "Enter number of channels (1-4): "
                                 (mapcar (lambda (x) (cons (format "%d" x) 
nil)) (iota 4 1))
                                 (function identity)
                                 t                 ; require match
                                 "" nil "1" nil)))

        (string= "yes"
                 (completing-read "Replace 0 StO$_2$ values with na: "
                                  (mapcar (lambda (x) (cons x nil)) '("yes" 
"no"))
                                  (function identity)
                                  t                 ; require match
                                  "" nil "yes" nil))))
      (barf-if-buffer-read-only)
      (save-excursion
        (replace-regexp " +" "," nil (point-min) (point-max))
        (goto-char (point-min))
        (let ((csv-separators '(",")))
          (csv-kill-fields (set-difference (iota 34 1)
                                           (ecase number-of-channels
                                             ((1) '(1 2 3))
                                             ((2) '(1 2 3 10))
                                             ((3) '(1 2 3 10 17))
                                             ((4) '(1 2 3 10 17 24))))
                           (point-min) (point-max)))
        (when replace-StO2-0-p
          (goto-char (point-min))
          (while (re-search-forward "[^[:digit:]]0[^[:digit:]]" nil t)
            (replace-match "na ")))
        ;; let's put back spaces as separators:
        (replace-regexp "," " " nil (point-min) (point-max))))




Finally, when you'll have completed the "Introduction to Emacs Lisp", I
would advise you to read SICP.

SICP   = Structure and Interpretation of Computer Programs
         http://mitpress.mit.edu/sicp/full-text/book/book-Z-H-4.html
         http://swiss.csail.mit.edu/classes/6.001/abelson-sussman-lectures/
         
http://www.codepoetics.com/wiki/index.php?title=Topics:SICP_in_other_languages
         http://eli.thegreenplace.net/category/programming/lisp/sicp/
         http://www.neilvandyke.org/sicp-plt/
         http://www.youtube.com/watch?v=rdj6deraQ6k

(and watch the videos, they're quite instructive).  

While this book uses scheme, it's subject matter is not about scheme,
but programming in general, as can be seen from the translations of the
exercises in other programming languages (well, it seems the wiki has
been defaced, so it's not available hopefully temporarily, but eli's
blog contains Common Lisp versions of the exercises, which are close to
emacs lisp).


-- 
__Pascal Bourguignon__                     http://www.informatimago.com/
A bad day in () is better than a good day in {}.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]