Improve similarity explanations [LUCENE-8011] about stargazers-migration-test HOT 15 CLOSED

Hello! What other specific similarity classes we would like to tackle here?

Are for example AfterEffect, AfterEffectB, Normalization be good candidates?

[Legacy Jira: Mayya Sharipova (@mayya-sharipova) on Nov 26 2017]

from stargazers-migration-test.

@mayyas It looks like there is room for improvement in those classes indeed.

In general I think doing the following in our core similarities, like BM25Similarity does, would be nice

• include the formula that was used to compute the score
• give back constructor args with a nice description (eg. NormalizationH1 does not explain its c parameter, and the Axiomatic similarity includes them in the description but with poor descriptions)

[Legacy Jira: Adrien Grand (@jpountz) on Nov 28 2017]

from stargazers-migration-test.

thanks @jpountz, will work on the classes you suggested

[Legacy Jira: Mayya Sharipova (@mayya-sharipova) on Nov 29 2017]

from stargazers-migration-test.

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 <[email protected]>
Date: 2017-12-01T01:03:39Z

LUCENE-8011: Improve similarity explanations

[Legacy Jira: ASF GitHub Bot on Dec 01 2017]

from stargazers-migration-test.

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.

[Legacy Jira: ASF GitHub Bot on Dec 01 2017]

from stargazers-migration-test.

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

[Legacy Jira: ASF GitHub Bot on Dec 01 2017]

from stargazers-migration-test.

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.

[Legacy Jira: ASF GitHub Bot on Dec 05 2017]

from stargazers-migration-test.

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.

[Legacy Jira: ASF GitHub Bot on Dec 05 2017]

from stargazers-migration-test.

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:`

[Legacy Jira: ASF GitHub Bot on Dec 07 2017]

from stargazers-migration-test.

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?

[Legacy Jira: ASF GitHub Bot on Dec 12 2017]

from stargazers-migration-test.

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.

[Legacy Jira: ASF GitHub Bot on Dec 12 2017]

from stargazers-migration-test.

Github user jpountz commented on the issue:

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

No need to be sorry!

[Legacy Jira: ASF GitHub Bot on Dec 12 2017]

from stargazers-migration-test.

Commit 6771886cf3303a9a2c3cb86c8d0329e55ee0feaa in lucene-solr's branch refs/heads/master from @mayya-sharipova
https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6771886

LUCENE-8011: Improve similarity explanations

Closes #280

Signed-off-by: Adrien Grand <[email protected]>

[Legacy Jira: ASF subversion and git services on Dec 13 2017]

from stargazers-migration-test.

Github user asfgit closed the pull request at:

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

[Legacy Jira: ASF GitHub Bot on Dec 13 2017]

from stargazers-migration-test.

Github user jpountz commented on the issue:

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

Merged, thank you @mayya-sharipova.

[Legacy Jira: ASF GitHub Bot on Dec 13 2017]

from stargazers-migration-test.