Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.2, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      We should make this API easier to consume, for instance:

      • enforce important components to be non-null (eg. description)
      • decouple entirely the score computation from whether there is a match or not (Explanation assumes there is a match if the score is > 0, you need to use ComplexExplanation to override this behaviour)
      • return an empty array instead of null when there are no "details"
      1. LUCENE-6446.patch
        113 kB
        Adrien Grand

        Activity

        Hide
        Adrien Grand added a comment -

        Here is a patch which removes ComplexExplanation and makes Explanation immutable.

        Show
        Adrien Grand added a comment - Here is a patch which removes ComplexExplanation and makes Explanation immutable.
        Hide
        Ryan Ernst added a comment -

        +1, this is much cleaner!

        Show
        Ryan Ernst added a comment - +1, this is much cleaner!
        Hide
        ASF subversion and git services added a comment -

        Commit 1675109 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1675109 ]

        LUCENE-6446: Simplified Explanation API.

        Show
        ASF subversion and git services added a comment - Commit 1675109 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1675109 ] LUCENE-6446 : Simplified Explanation API.
        Hide
        ASF subversion and git services added a comment -

        Commit 1675114 from Adrien Grand in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1675114 ]

        LUCENE-6446: Simplified Explanation API.

        Show
        ASF subversion and git services added a comment - Commit 1675114 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1675114 ] LUCENE-6446 : Simplified Explanation API.
        Hide
        Adrien Grand added a comment -

        Thanks Ryan!

        Show
        Adrien Grand added a comment - Thanks Ryan!
        Hide
        ASF subversion and git services added a comment -

        Commit 1675132 from Adrien Grand in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1675132 ]

        LUCENE-6446: Fix explanations of BM25Similarity.

        Show
        ASF subversion and git services added a comment - Commit 1675132 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1675132 ] LUCENE-6446 : Fix explanations of BM25Similarity.
        Hide
        ASF subversion and git services added a comment -

        Commit 1675133 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1675133 ]

        LUCENE-6446: Fix explanations of BM25Similarity.

        Show
        ASF subversion and git services added a comment - Commit 1675133 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1675133 ] LUCENE-6446 : Fix explanations of BM25Similarity.
        Hide
        ASF subversion and git services added a comment -

        Commit 1675152 from Adrien Grand in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1675152 ]

        LUCENE-6446: Protected against null sub explanations.

        Show
        ASF subversion and git services added a comment - Commit 1675152 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1675152 ] LUCENE-6446 : Protected against null sub explanations.
        Hide
        ASF subversion and git services added a comment -

        Commit 1675153 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1675153 ]

        LUCENE-6446: Protected against null sub explanations.

        Show
        ASF subversion and git services added a comment - Commit 1675153 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1675153 ] LUCENE-6446 : Protected against null sub explanations.
        Hide
        Terry Smith added a comment -

        The refactored Explanation looks great, however I see a couple of small issues worth raising.

        1. The constructor is private and there is a protected toString(int depth) method, it doesn't look like anyone else is calling it and no-one can subclass it. Should this method be private?

        2. The toString() output is different! ComplexExplanation had a slightly different getSummary() method:

            return getValue() + " = "
              + (isMatch() ? "(MATCH) " : "(NON-MATCH) ")
              + getDescription();
        

        versus

            return getValue() + " = " + getDescription();
        

        I find this extra context invaluable, especially with the decoupling of score and match, we can't assume that a score of 0 is a NON-MATCH yet the output no longer tells is if an explanation is a MATCH or not.

        I understand that I can roll my own string building code with the current API. It'd be great if the default output was as useful as possible.

        Show
        Terry Smith added a comment - The refactored Explanation looks great, however I see a couple of small issues worth raising. 1. The constructor is private and there is a protected toString(int depth) method, it doesn't look like anyone else is calling it and no-one can subclass it. Should this method be private? 2. The toString() output is different! ComplexExplanation had a slightly different getSummary() method: return getValue() + " = " + (isMatch() ? "(MATCH) " : "(NON-MATCH) " ) + getDescription(); versus return getValue() + " = " + getDescription(); I find this extra context invaluable, especially with the decoupling of score and match, we can't assume that a score of 0 is a NON-MATCH yet the output no longer tells is if an explanation is a MATCH or not. I understand that I can roll my own string building code with the current API. It'd be great if the default output was as useful as possible.
        Hide
        Adrien Grand added a comment -

        Should this method be private?

        Good point, I'll fix.

        I find this extra context invaluable, especially with the decoupling of score and match, we can't assume that a score of 0 is a NON-MATCH yet the output no longer tells is if an explanation is a MATCH or not.

        I removed it because it was not always in the summary (only when using ComplexExplanation) as well as redundant with the description which is explicit when there is no match, for instance TermWeight's "no matching term" or BooleanWeight "no match on required clause"?

        Show
        Adrien Grand added a comment - Should this method be private? Good point, I'll fix. I find this extra context invaluable, especially with the decoupling of score and match, we can't assume that a score of 0 is a NON-MATCH yet the output no longer tells is if an explanation is a MATCH or not. I removed it because it was not always in the summary (only when using ComplexExplanation) as well as redundant with the description which is explicit when there is no match, for instance TermWeight's "no matching term" or BooleanWeight "no match on required clause"?
        Hide
        Terry Smith added a comment -

        I removed it because it was not always in the summary (only when using ComplexExplanation) as well as redundant with the description which is explicit when there is no match, for instance TermWeight's "no matching term" or BooleanWeight "no match on required clause"?

        That makes sense. Removing the redundant information is definitely the way to go.

        I also noticed that the new Explanation.noMatch() methods look a little trappy. They both take the child details and drop them on the floor.

          public static Explanation noMatch(String description, Collection<Explanation> details) {
            return new Explanation(false, 0f, description, Collections.emptyList());
          }
        

        I think the noMatch() methods should either add the details to the created explanation or not accept them as parameters. Having a non-matching explanation contain child details can be really useful for complex queries. What do you think?

        Show
        Terry Smith added a comment - I removed it because it was not always in the summary (only when using ComplexExplanation) as well as redundant with the description which is explicit when there is no match, for instance TermWeight's "no matching term" or BooleanWeight "no match on required clause"? That makes sense. Removing the redundant information is definitely the way to go. I also noticed that the new Explanation.noMatch() methods look a little trappy. They both take the child details and drop them on the floor. public static Explanation noMatch( String description, Collection<Explanation> details) { return new Explanation( false , 0f, description, Collections.emptyList()); } I think the noMatch() methods should either add the details to the created explanation or not accept them as parameters. Having a non-matching explanation contain child details can be really useful for complex queries. What do you think?
        Hide
        Adrien Grand added a comment -

        Oh yes, that's a bad bug! Thanks for catching this!

        Show
        Adrien Grand added a comment - Oh yes, that's a bad bug! Thanks for catching this!
        Hide
        ASF subversion and git services added a comment -

        Commit 1675363 from Adrien Grand in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1675363 ]

        LUCENE-6446: Fix method visibility and trappy factory method.

        Show
        ASF subversion and git services added a comment - Commit 1675363 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1675363 ] LUCENE-6446 : Fix method visibility and trappy factory method.
        Hide
        ASF subversion and git services added a comment -

        Commit 1675365 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1675365 ]

        LUCENE-6446: Fix method visibility and trappy factory method.

        Show
        ASF subversion and git services added a comment - Commit 1675365 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1675365 ] LUCENE-6446 : Fix method visibility and trappy factory method.
        Hide
        Anshum Gupta added a comment -

        Bulk close for 5.2.0.

        Show
        Anshum Gupta added a comment - Bulk close for 5.2.0.

          People

          • Assignee:
            Adrien Grand
            Reporter:
            Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development