* Re: [PATCH] Support time units in est+
2017-01-18 20:33 [PATCH] Support time units in est+ Malcolm Matalka
@ 2017-01-18 21:13 ` Nick Dokos
[not found] ` <CAKziXDUDhHJWmfm2kN+tM2WmXPiqEXTfZtQYVriCBydRqiiW7g@mail.gmail.com>
2017-02-03 12:56 ` Malcolm Matalka
2017-02-04 14:30 ` Nicolas Goaziou
2 siblings, 1 reply; 6+ messages in thread
From: Nick Dokos @ 2017-01-18 21:13 UTC (permalink / raw)
To: emacs-orgmode
Malcolm Matalka <mmatalka@gmail.com> writes:
> Hey, this is my first elisp programming so I'm quite certain this is a
> big hack. I just stole elements from elsewhere in the file. I'm hoping
> this is good enough to get accepted then perhaps someone with more taste
> would be able to refactor it to be a bit better.
>
> Let me know if I need to change anything.
>
Yes, indeed: the first thing you should do is explain what you are
trying to do. E.g. what does "est+" mean? Once you have explained
*what* you are trying to do, then somebody might be able to suggest
*how* to do it better (if it is a worthwhile thing to do in the first place).
> From 1167bd20e042ad2ae3f2712f596d76ad8b230336 Mon Sep 17 00:00:00 2001
> From: orbitz <orbitz@gmail.com>
> Date: Wed, 18 Jan 2017 21:18:23 +0100
> Subject: [PATCH] org-colview.el: Add support for time units to est+
>
> * lisp/org-colview.el: Add support for time units in est+. Ranges are interpreted just like non-range estimates.
>
> TINYCHANGE
> ---
> lisp/org-colview.el | 44 ++++++++++++++++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/lisp/org-colview.el b/lisp/org-colview.el
> index 45c71a028..2a5c067ac 100644
> --- a/lisp/org-colview.el
> +++ b/lisp/org-colview.el
> @@ -1288,23 +1288,43 @@ When PRINTF is non-nil, use it to format the result."
> (/ (apply #'+ (mapcar #'org-columns--age-to-seconds ages))
> (float (length ages)))))
>
> -(defun org-columns--summary-estimate (estimates printf)
> +(defun org-columns--summary-estimate (estimates _)
> "Combine a list of estimates, using mean and variance.
> The mean and variance of the result will be the sum of the means
> and variances (respectively) of the individual estimates."
> (let ((mean 0)
> - (var 0))
> + (var 0)
> + (hms-flag nil)
> + (duration-flag nil))
> (dolist (e estimates)
> - (pcase (mapcar #'string-to-number (split-string e "-"))
> -(`(,low ,high)
> - (let ((m (/ (+ low high) 2.0)))
> - (cl-incf mean m)
> - (cl-incf var (- (/ (+ (* low low) (* high high)) 2.0) (* m m)))))
> -(`(,value) (cl-incf mean value))))
> - (let ((sd (sqrt var)))
> - (format "%s-%s"
> - (format (or printf "%.0f") (- mean sd))
> - (format (or printf "%.0f") (+ mean sd))))))
> + (pcase (split-string e "-")
> + (`(,low ,high)
> + (dolist (time (list high low))
> + (cond
> + (duration-flag)
> + ((string-match-p org-columns--duration-re time)
> + (setq duration-flag t))
> + (hms-flag)
> + ((string-match-p "\\`[0-9]+:[0-9]+:[0-9]+\\'" time)
> + (setq hms-flag t))))
> + (let* ((low-sec (org-columns--time-to-seconds low))
> + (high-sec (org-columns--time-to-seconds high))
> + (m (/ (+ low-sec high-sec) 2.0)))
> + (cl-incf mean m)
> + (cl-incf var (- (/ (+ (* low-sec low-sec) (* high-sec high-sec)) 2.0) (* m m)))))
> + (`(,value) (cl-incf mean (org-columns--time-to-seconds value)))))
> + (let* ((sd (sqrt var))
> + (low-second (truncate (- mean sd)))
> + (high-second (truncate (+ mean sd)))
> + (low
> + (cond (duration-flag (org-minutes-to-clocksum-string (/ low-second 60.0)))
> + (hms-flag (format-seconds "%h:%.2m:%.2s" low-second))
> + (t (format-seconds "%h:%.2m" low-second))))
> + (high
> + (cond (duration-flag (org-minutes-to-clocksum-string (/ high-second 60.0)))
> + (hms-flag (format-seconds "%h:%.2m:%.2s" high-second))
> + (t (format-seconds "%h:%.2m" high-second)))))
> + (format "%s-%s" low high))))
--
Nick
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Support time units in est+
2017-01-18 20:33 [PATCH] Support time units in est+ Malcolm Matalka
2017-01-18 21:13 ` Nick Dokos
@ 2017-02-03 12:56 ` Malcolm Matalka
2017-02-04 14:30 ` Nicolas Goaziou
2 siblings, 0 replies; 6+ messages in thread
From: Malcolm Matalka @ 2017-02-03 12:56 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 3612 bytes --]
Hello, has anyone had an opportunity to review this? Thank you.
Den 18 jan. 2017 9:33 em skrev "Malcolm Matalka" <mmatalka@gmail.com>:
> Hey, this is my first elisp programming so I'm quite certain this is a
> big hack. I just stole elements from elsewhere in the file. I'm hoping
> this is good enough to get accepted then perhaps someone with more taste
> would be able to refactor it to be a bit better.
>
> Let me know if I need to change anything.
>
> From 1167bd20e042ad2ae3f2712f596d76ad8b230336 Mon Sep 17 00:00:00 2001
> From: orbitz <orbitz@gmail.com>
> Date: Wed, 18 Jan 2017 21:18:23 +0100
> Subject: [PATCH] org-colview.el: Add support for time units to est+
>
> * lisp/org-colview.el: Add support for time units in est+. Ranges are
> interpreted just like non-range estimates.
>
> TINYCHANGE
> ---
> lisp/org-colview.el | 44 ++++++++++++++++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/lisp/org-colview.el b/lisp/org-colview.el
> index 45c71a028..2a5c067ac 100644
> --- a/lisp/org-colview.el
> +++ b/lisp/org-colview.el
> @@ -1288,23 +1288,43 @@ When PRINTF is non-nil, use it to format the
> result."
> (/ (apply #'+ (mapcar #'org-columns--age-to-seconds ages))
> (float (length ages)))))
>
> -(defun org-columns--summary-estimate (estimates printf)
> +(defun org-columns--summary-estimate (estimates _)
> "Combine a list of estimates, using mean and variance.
> The mean and variance of the result will be the sum of the means
> and variances (respectively) of the individual estimates."
> (let ((mean 0)
> - (var 0))
> + (var 0)
> + (hms-flag nil)
> + (duration-flag nil))
> (dolist (e estimates)
> - (pcase (mapcar #'string-to-number (split-string e "-"))
> -(`(,low ,high)
> - (let ((m (/ (+ low high) 2.0)))
> - (cl-incf mean m)
> - (cl-incf var (- (/ (+ (* low low) (* high high)) 2.0) (* m m)))))
> -(`(,value) (cl-incf mean value))))
> - (let ((sd (sqrt var)))
> - (format "%s-%s"
> - (format (or printf "%.0f") (- mean sd))
> - (format (or printf "%.0f") (+ mean sd))))))
> + (pcase (split-string e "-")
> + (`(,low ,high)
> + (dolist (time (list high low))
> + (cond
> + (duration-flag)
> + ((string-match-p org-columns--duration-re time)
> + (setq duration-flag t))
> + (hms-flag)
> + ((string-match-p "\\`[0-9]+:[0-9]+:[0-9]+\\'" time)
> + (setq hms-flag t))))
> + (let* ((low-sec (org-columns--time-to-seconds low))
> + (high-sec (org-columns--time-to-seconds high))
> + (m (/ (+ low-sec high-sec) 2.0)))
> + (cl-incf mean m)
> + (cl-incf var (- (/ (+ (* low-sec low-sec) (* high-sec
> high-sec)) 2.0) (* m m)))))
> + (`(,value) (cl-incf mean (org-columns--time-to-seconds value)))))
> + (let* ((sd (sqrt var))
> + (low-second (truncate (- mean sd)))
> + (high-second (truncate (+ mean sd)))
> + (low
> + (cond (duration-flag (org-minutes-to-clocksum-string (/
> low-second 60.0)))
> + (hms-flag (format-seconds "%h:%.2m:%.2s" low-second))
> + (t (format-seconds "%h:%.2m" low-second))))
> + (high
> + (cond (duration-flag (org-minutes-to-clocksum-string (/
> high-second 60.0)))
> + (hms-flag (format-seconds "%h:%.2m:%.2s" high-second))
> + (t (format-seconds "%h:%.2m" high-second)))))
> + (format "%s-%s" low high))))
>
>
>
> --
> 2.11.0
>
>
[-- Attachment #2: Type: text/html, Size: 4573 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Support time units in est+
2017-01-18 20:33 [PATCH] Support time units in est+ Malcolm Matalka
2017-01-18 21:13 ` Nick Dokos
2017-02-03 12:56 ` Malcolm Matalka
@ 2017-02-04 14:30 ` Nicolas Goaziou
2 siblings, 0 replies; 6+ messages in thread
From: Nicolas Goaziou @ 2017-02-04 14:30 UTC (permalink / raw)
To: Malcolm Matalka; +Cc: emacs-orgmode
Hello,
Malcolm Matalka <mmatalka@gmail.com> writes:
> Hey, this is my first elisp programming so I'm quite certain this is a
> big hack. I just stole elements from elsewhere in the file. I'm hoping
> this is good enough to get accepted then perhaps someone with more taste
> would be able to refactor it to be a bit better.
That's not bad. Thank you.
> Let me know if I need to change anything.
>
> From 1167bd20e042ad2ae3f2712f596d76ad8b230336 Mon Sep 17 00:00:00 2001
> From: orbitz <orbitz@gmail.com>
> Date: Wed, 18 Jan 2017 21:18:23 +0100
> Subject: [PATCH] org-colview.el: Add support for time units to est+
>
> * lisp/org-colview.el: Add support for time units in est+. Ranges are interpreted just like non-range estimates.
You need to provide the name of the function being modified:
* lisp/org-colview.el (org-columns--summary-estimate): Add support...
>
> TINYCHANGE
> ---
> lisp/org-colview.el | 44 ++++++++++++++++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/lisp/org-colview.el b/lisp/org-colview.el
> index 45c71a028..2a5c067ac 100644
> --- a/lisp/org-colview.el
> +++ b/lisp/org-colview.el
> @@ -1288,23 +1288,43 @@ When PRINTF is non-nil, use it to format the result."
> (/ (apply #'+ (mapcar #'org-columns--age-to-seconds ages))
> (float (length ages)))))
>
> -(defun org-columns--summary-estimate (estimates printf)
> +(defun org-columns--summary-estimate (estimates _)
> "Combine a list of estimates, using mean and variance.
> The mean and variance of the result will be the sum of the means
> and variances (respectively) of the individual estimates."
Could you expound the docstring? In particular, it should make the
output format predictable. See `org-columns--summary-apply-times' for
example.
> (let ((mean 0)
> - (var 0))
> + (var 0)
> + (hms-flag nil)
> + (duration-flag nil))
> (dolist (e estimates)
> - (pcase (mapcar #'string-to-number (split-string e "-"))
> -(`(,low ,high)
> - (let ((m (/ (+ low high) 2.0)))
> - (cl-incf mean m)
> - (cl-incf var (- (/ (+ (* low low) (* high high)) 2.0) (* m m)))))
> -(`(,value) (cl-incf mean value))))
> - (let ((sd (sqrt var)))
> - (format "%s-%s"
> - (format (or printf "%.0f") (- mean sd))
> - (format (or printf "%.0f") (+ mean sd))))))
> + (pcase (split-string e "-")
> + (`(,low ,high)
> + (dolist (time (list high low))
> + (cond
> + (duration-flag)
> + ((string-match-p org-columns--duration-re time)
> + (setq duration-flag t))
> + (hms-flag)
> + ((string-match-p "\\`[0-9]+:[0-9]+:[0-9]+\\'" time)
> + (setq hms-flag t))))
> + (let* ((low-sec (org-columns--time-to-seconds low))
> + (high-sec (org-columns--time-to-seconds high))
> + (m (/ (+ low-sec high-sec) 2.0)))
> + (cl-incf mean m)
> + (cl-incf var (- (/ (+ (* low-sec low-sec) (* high-sec high-sec)) 2.0) (* m m)))))
> + (`(,value) (cl-incf mean (org-columns--time-to-seconds
> value)))))
There is much code duplication with `org-columns--summary-apply-times'.
It would be better to avoid it.
We may want to create a new function turning a list of durations into
numbers (with `org-columns--time-to-seconds) and adding the type of the
expected output, as a symbol, in front of the list. Then both
`org-columns--summary-estimate' and `org-columns--summary-apply-times'
could use it.
It would mean we would process the list of estimates two times, but
I don't think it matters much in practice.
If you have an implementation for that, feel free to include it. Do not
consider it a blocker, tho. We can always factor this out later.
> + (let* ((sd (sqrt var))
> + (low-second (truncate (- mean sd)))
> + (high-second (truncate (+ mean sd)))
> + (low
> + (cond (duration-flag (org-minutes-to-clocksum-string (/ low-second 60.0)))
> + (hms-flag (format-seconds "%h:%.2m:%.2s" low-second))
> + (t (format-seconds "%h:%.2m" low-second))))
> + (high
> + (cond (duration-flag (org-minutes-to-clocksum-string (/ high-second 60.0)))
> + (hms-flag (format-seconds "%h:%.2m:%.2s" high-second))
> + (t (format-seconds "%h:%.2m" high-second)))))
> + (format "%s-%s" low high))))
You don't need to `truncate' the estimates.
I don't think the default output format should be "%h:%.2m". If no
special format is present, I think the traditional "%.0f" can be used.
Also, the duplicate `cond' can be factored out. The whole part above can
be written (including suggestions aforementioned):
(mapconcat (lambda (time)
(cond
(duration-flag (org-minutes-to-clocksum-string (/ time 60)))
(hms-flag (format-seconds "%h:%.2m:%.2s" time))
(t (format "%.0f" time))))
(let ((sd (sqrt var))) (list (- mean sd) (+ mean sd)))
"-")
It would be nice to add a bunch of tests for various estimates formats.
`test-org-colview/columns-summary' in "test-org-colview.el" contains
already many examples to boot.
Eventually, could you add an entry about this new feature in ORG-NEWS?
Regards,
--
Nicolas Goaziou
^ permalink raw reply [flat|nested] 6+ messages in thread