bug#36763: `guix search` does unexpected logical and

bug#36763: `guix search` does unexpected logical and

[zimoun]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

Dear,

As discussed here [1], the `relevance` in `guix/ui.scm` does not match
"inter-field".

Attached a fix.
Now,  the example from the manual
  $ guix search crypto library | \
        recsel -e '! (name ~ "^(ghc|perl|python|ruby)")' -p name,synopsis
outputs the expected crypto libraries as `libb2`.


Please comment. :-)


Then, please indicate me how the commit has to be filled.
The commit 8874faaaac665100a095ef25e39c9a389f5a397f introducing the
logical AND says:

ui: 'relevance' considers regexps connected with a logical and.

* guix/ui.scm (relevance)[score]: Change to return 0 when one of REGEXPS
doesn't match.
* tests/ui.scm ("package-relevance"): New test.

Should another test be added?


[1] https://lists.gnu.org/archive/html/guix-devel/2019-07/msg00263.html


Thank you in advance,
simon

[-- Attachment #2: search.patch --]
[-- Type: text/x-patch, Size: 2101 bytes --]

diff --git a/guix/ui.scm b/guix/ui.scm
index 7920335928..0e60eb6edc 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -1291,23 +1291,36 @@ score, the more relevant OBJ is to REGEXPS."
                                     5             ;exact match
                                     1)))))
            regexps))
-
+    scores)
+
+  (define (update relevance weight scores)
+    (map + relevance
+           (map (lambda (score)
+                  (* weight score))
+                scores)))
+
+  (let ((scores (fold (lambda (metric relevance)
+                        (match metric
+                          ((field . weight)
+                           (match (field obj)
+                             (#f  relevance)
+                             ((? string? str)
+                              (update relevance weight (score str)))
+                             ((lst ...)
+                              (update relevance weight
+                                      (fold (lambda (elem prev)
+                                              (if (zero? (length elem))
+                                                  prev
+                                                  (map + elem prev)))
+                                            (make-list (length regexps) 0)
+                                            (map score lst)))
+                              )))))
+                      (make-list (length regexps) 0)
+                      metrics)))
     ;; Return zero if one of REGEXPS doesn't match.
     (if (any zero? scores)
         0
-        (reduce + 0 scores)))
-
-  (fold (lambda (metric relevance)
-          (match metric
-            ((field . weight)
-             (match (field obj)
-               (#f  relevance)
-               ((? string? str)
-                (+ relevance (* (score str) weight)))
-               ((lst ...)
-                (+ relevance (* weight (apply + (map score lst)))))))))
-        0
-        metrics))
+        (reduce + 0 scores))))
 
 (define %package-metrics
   ;; Metrics used to compute the "relevance score" of a package against a set

bug#36763: `guix search` does unexpected logical and

[Ludovic Courtès]

Hi Simon,

zimoun <zimon.toutoune@gmail.com> skribis:

> As discussed here [1], the `relevance` in `guix/ui.scm` does not match
> "inter-field".
>
> Attached a fix.
> Now,  the example from the manual
>   $ guix search crypto library | \
>         recsel -e '! (name ~ "^(ghc|perl|python|ruby)")' -p name,synopsis
> outputs the expected crypto libraries as `libb2`.

Nice!

> Then, please indicate me how the commit has to be filled.
> The commit 8874faaaac665100a095ef25e39c9a389f5a397f introducing the
> logical AND says:
>
> ui: 'relevance' considers regexps connected with a logical and.
>
> * guix/ui.scm (relevance)[score]: Change to return 0 when one of REGEXPS
> doesn't match.
> * tests/ui.scm ("package-relevance"): New test.

Yes, you can provide a commit log along these lines.

> Should another test be added?

Yes please, that would be perfect!

> +  (define (update relevance weight scores)
> +    (map + relevance
> +           (map (lambda (score)
> +                  (* weight score))
> +                scores)))
> +
> +  (let ((scores (fold (lambda (metric relevance)
> +                        (match metric
> +                          ((field . weight)
> +                           (match (field obj)
> +                             (#f  relevance)
> +                             ((? string? str)
> +                              (update relevance weight (score str)))
> +                             ((lst ...)
> +                              (update relevance weight
> +                                      (fold (lambda (elem prev)
> +                                              (if (zero? (length elem))
> +                                                  prev
> +                                                  (map + elem prev)))
> +                                            (make-list (length regexps) 0)
> +                                            (map score lst)))
> +                              )))))
> +                      (make-list (length regexps) 0)
> +                      metrics)))

I don’t have a clear mind on that, but I feel like this could be
simplified somehow.  For instance,

  (fold p (make-list …) metrics)

looks a lot like:

  (map (lambda (x) (fold p x metrics)) regexps)

Well, something like that.  :-)

Thanks for working on it!

Ludo’.

bug#36763: `guix search` does unexpected logical and

[zimoun]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

Hi Ludo,

Thank you to look at it.

On Thu, 25 Jul 2019 at 19:35, Ludovic Courtès <ludo@gnu.org> wrote:

>   (fold p (make-list …) metrics)
>
> looks a lot like:
>
>   (map (lambda (x) (fold p x metrics)) regexps)
>
> Well, something like that.  :-)

Second try attached with your advice. :-)


I am not sure where to place the newlines on long line.
Then I will try to add tests and send it.


Thank you.


All the best,
simon

[-- Attachment #2: search.patch2 --]
[-- Type: application/octet-stream, Size: 2473 bytes --]

diff --git a/guix/ui.scm b/guix/ui.scm
index 7920335928..80b92bcc5f 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -1281,33 +1281,37 @@ weight of this field in the final score.
 
 A score of zero means that OBJ does not match any of REGEXPS.  The higher the
 score, the more relevant OBJ is to REGEXPS."
-  (define (score str)
-    (define scores
-      (map (lambda (regexp)
-             (fold-matches regexp str 0
-                           (lambda (m score)
-                             (+ score
-                                (if (string=? (match:substring m) str)
-                                    5             ;exact match
-                                    1)))))
-           regexps))
+  (define (score str regexp)
+    (fold-matches regexp str 0
+                  (lambda (m score)
+                    (+ score
+                       (if (string=? (match:substring m) str)
+                           5             ;exact match
+                           1)))))
+
+  (let ((scores (map
+                 (lambda (regexp)
+                   (fold
+                    (lambda (metric relevance)
+                      (match metric
+                        ((field . weight)
+                         (match (field obj)
+                           (#f  relevance)
+                           ((? string? str)
+                            (+ relevance (* (score str regexp) weight)))
+                           ((lst ...)
+                            (+ relevance (* weight
+                                            (apply + (map
+                                                      (lambda (str)
+                                                        (score str regexp))
+                                                      lst)))))))))
+                    0 metrics))
+                 regexps)))
 
     ;; Return zero if one of REGEXPS doesn't match.
     (if (any zero? scores)
         0
-        (reduce + 0 scores)))
-
-  (fold (lambda (metric relevance)
-          (match metric
-            ((field . weight)
-             (match (field obj)
-               (#f  relevance)
-               ((? string? str)
-                (+ relevance (* (score str) weight)))
-               ((lst ...)
-                (+ relevance (* weight (apply + (map score lst)))))))))
-        0
-        metrics))
+        (reduce + 0 scores))))
 
 (define %package-metrics
   ;; Metrics used to compute the "relevance score" of a package against a set

bug#36763: `guix search` does unexpected logical and

[Ludovic Courtès]

Hello!

zimoun <zimon.toutoune@gmail.com> skribis:

> On Thu, 25 Jul 2019 at 19:35, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>   (fold p (make-list …) metrics)
>>
>> looks a lot like:
>>
>>   (map (lambda (x) (fold p x metrics)) regexps)
>>
>> Well, something like that.  :-)
>
> Second try attached with your advice. :-)

Neat!

> +  (let ((scores (map
> +                 (lambda (regexp)
> +                   (fold
> +                    (lambda (metric relevance)
> +                      (match metric
> +                        ((field . weight)
> +                         (match (field obj)
> +                           (#f  relevance)
> +                           ((? string? str)
> +                            (+ relevance (* (score str regexp) weight)))
> +                           ((lst ...)
> +                            (+ relevance (* weight
> +                                            (apply + (map
> +                                                      (lambda (str)
> +                                                        (score str regexp))
> +                                                      lst)))))))))
> +                    0 metrics))
> +                 regexps)))

For readability, I’d suggest giving a name to one of the two lambdas
above, so you can write, say:

  (map regexp-scores regexps)

where:

  (define (regexp-scores regexp)
    (fold (lambda (metric relevance)
            …)
          …))

Also, could you add a couple of tests (such as the “libb2” example for
“crypto” + “library” you mentioned earlier)?  You can add them to the
existing “package-relevance” test in tests/ui.scm.

Bonus points if you send the patch with ‘git format-patch’ and with a
commit log:

  https://guix.gnu.org/manual/en/html_node/Submitting-Patches.html

Thank you!

Ludo’.

bug#36763: `guix search` does unexpected logical and

[zimoun]

Hi Ludo,

On Mon, 16 Sep 2019 at 10:17, Ludovic Courtès <ludo@gnu.org> wrote:

> Bonus points if you send the patch with ‘git format-patch’ and with a
> commit log:

I did my best here:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37448

Hope I am doing right.

Thanks,
simon

bug#36763: `guix search` does unexpected logical and

[Ludovic Courtès]

Hello,

zimoun <zimon.toutoune@gmail.com> skribis:

> On Mon, 16 Sep 2019 at 10:17, Ludovic Courtès <ludo@gnu.org> wrote:
>
>> Bonus points if you send the patch with ‘git format-patch’ and with a
>> commit log:
>
> I did my best here:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37448
>
> Hope I am doing right.

Yup!

We can close it now, thank you.

Ludo’.