[GitHub] lucene-solr pull request #280: LUCENE-8011: Improve similarity explanations

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #280: LUCENE-8011: Improve similarity explanations

asfgit
GitHub user mayya-sharipova opened a pull request:

    https://github.com/apache/lucene-solr/pull/280

    LUCENE-8011: Improve similarity explanations

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/mayya-sharipova/lucene-solr LUCENE-8011-improve-similarity-explanations

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/lucene-solr/pull/280.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #280
   
----
commit c389c4992b66b5ae750ba7aa5b37937ebedc6615
Author: Mayya Sharipova <[hidden email]>
Date:   2017-12-01T01:03:39Z

    LUCENE-8011: Improve similarity explanations

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
Github user jpountz commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    Thanks @mayya-sharipova, this looks like great progress to me. Maybe we could go even further and do the following:
     - in the Axiomatic similarity, add abstract methods to allow sub classes to explain how tf, ln, etc. are computed,
     - make BasicModel.explain abstract to force sub classes to have their own explanation and include the formula,
     - make sure that our own sub classes of SimilarityBase extend explain (the one that returns an explanation) and include the formula in the explanation.
   
    For the record, there is not too much concern to have about backward compatibility since most of those classes (eg. Axiomatic, BasicModel) are very expert classes and this changes targets master.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user mayya-sharipova commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    @jpountz thank you Adrien, I will work on these classes as well


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user jpountz commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    Thank you Mayya, it's much easier to see where these scores come from now. I tried running tests on your PR, but I'm getting failures. It seems to be due to the fact that some of the explanations that you added look like `computed as x from: ` while the test framework expects `computed as x from:` (no trailing whitespace). Removing these trailing whitespaces in explanations should fix the issue.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user mayya-sharipova commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    @jpountz Thanks, Adrien, sorry for that.  I will correct this, and next time make sure to run the tests before submitting a PR.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user jpountz commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/280#discussion_r155453742
 
    --- Diff: lucene/core/src/java/org/apache/lucene/search/similarities/AfterEffectL.java ---
    @@ -34,11 +34,14 @@ public final double score(BasicStats stats, double tfn) {
       }
       
       @Override
    +  // TODO: add explanation for tfn
    +  // Currently not possible, as CheckHits.verifyExplanation fails because
    +  // in case of a single sub-expl the test expects
    +  // the sub-expl's score to be equal to the parent expl's score
    --- End diff --
   
    this should be possible by rebasing or merging master back, I modified CheckHits yesterday so that it allows the score to be different from the parent explanation if the explanation matches `.*, computed as .* from:`


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user jpountz commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    Thanks @mayya-sharipova, this looks great. `ant precommit` complains from some missing docs (the build requires that all public/protected APIs have some minimal documentation), could you fix it?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user mayya-sharipova commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    @jpountz Adrien thanks for your help. Sorry, I will make sure to run `ant precommit` before committing next time. I have pushed another change to address this.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user jpountz commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    No need to be sorry!


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user asfgit closed the pull request at:

    https://github.com/apache/lucene-solr/pull/280


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #280: LUCENE-8011: Improve similarity explanations

asfgit
In reply to this post by asfgit
Github user jpountz commented on the issue:

    https://github.com/apache/lucene-solr/pull/280
 
    Merged, thank you @mayya-sharipova.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]