PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants, CHANGES.txt entries #13845

cpoerschke · 2024-10-01T17:17:30Z

#13757 follow-up

…iants

cpoerschke · 2024-10-01T17:17:46Z

Thanks @psalagnac for identifying this in the https://issues.apache.org/jira/browse/SOLR-17471 ticket!

cpoerschke · 2024-10-01T17:19:59Z

lucene/core/src/java/org/apache/lucene/search/similarities/Axiomatic.java

+   * @param queryLen the query length
+   * @param k hyperparam for the primitive weighting function
+   */
+  public Axiomatic(boolean discountOverlaps, float s, int queryLen, float k) {


Technically additional variant constructors could also be added for all the classes that derive from Axiomatic but there are relatively many derivations with multiple constructors. The class is marked @lucene.experimental and so perhaps it's okay to add additional variants if/when a need for them is identified?

cpoerschke · 2024-10-01T17:21:18Z

lucene/core/src/java/org/apache/lucene/search/similarities/DFISimilarity.java

+   * @param independenceMeasure measure of divergence from independence
+   * @param discountOverlaps true if overlap tokens should not impact document length for scoring.
+   */
+  public DFISimilarity(Independence independenceMeasure, boolean discountOverlaps) {


to support replacement of this: https://github.com/apache/solr/blob/releases/solr/9.7.0/solr/core/src/java/org/apache/solr/search/similarities/DFISimilarityFactory.java#L62-L63

cpoerschke · 2024-10-01T17:22:21Z

lucene/core/src/java/org/apache/lucene/search/similarities/DFRSimilarity.java

@@ -98,7 +98,7 @@ public DFRSimilarity(
  }

  /**
-   * Creates DFRSimilarity from the three components.
+   * Creates DFRSimilarity from the four components.


to support replacement of this: https://github.com/apache/solr/blob/releases/solr/9.7.0/solr/core/src/java/org/apache/solr/search/similarities/DFRSimilarityFactory.java#L163-L164

this isn't a DFR component. it is just an expert boolean.

when we use "component" in these docs, it has special meaning to the user. Let's make sure to not anything about that in these constructors (DFR is not the only one like this)

DFR models are obtained by instantiating the three components of the framework: selecting a basic randomness model, applying the first normalisation, and normalising the term frequencies

https://en.wikipedia.org/wiki/Divergence-from-randomness_model

cpoerschke · 2024-10-01T17:23:06Z

lucene/core/src/java/org/apache/lucene/search/similarities/IBSimilarity.java

+      Distribution distribution,
+      Lambda lambda,
+      Normalization normalization,
+      boolean discountOverlaps) {


to support replacement of this: https://github.com/apache/solr/blob/releases/solr/9.7.0/solr/core/src/java/org/apache/solr/search/similarities/IBSimilarityFactory.java#L103-L104

same as DFR, this isn't a component

done: reworded to ... three components and with the specified discountOverlaps value.

cpoerschke · 2024-10-01T17:24:45Z

lucene/core/src/java/org/apache/lucene/search/similarities/LMDirichletSimilarity.java

+  }
+
+  /** Instantiates the similarity with the provided parameters. */
+  public LMDirichletSimilarity(boolean discountOverlaps, float mu) {


to support replacement of this: https://github.com/apache/solr/blob/releases/solr/9.7.0/solr/core/src/java/org/apache/solr/search/similarities/LMDirichletSimilarityFactory.java#L54-L56

we only need one ctor here with this parameter, just add it to the "expert" ctor only.

cpoerschke · 2024-10-01T17:24:52Z

lucene/core/src/java/org/apache/lucene/search/similarities/LMDirichletSimilarity.java

@@ -62,8 +74,15 @@ public LMDirichletSimilarity(CollectionModel collectionModel) {
  }

  /** Instantiates the similarity with the default &mu; value of 2000. */
+  public LMDirichletSimilarity(boolean discountOverlaps) {


to support replacement of this: https://github.com/apache/solr/blob/releases/solr/9.7.0/solr/core/src/java/org/apache/solr/search/similarities/LMDirichletSimilarityFactory.java#L54-L56

add to expert CTOR only. no telescoping for this parameter, it is very expert.

cpoerschke · 2024-10-01T17:25:32Z

lucene/core/src/java/org/apache/lucene/search/similarities/LMJelinekMercerSimilarity.java

+  }
+
+  /** Instantiates with the specified parameters. */
+  public LMJelinekMercerSimilarity(boolean discountOverlaps, float lambda) {


to support replacement of this: https://github.com/apache/solr/blob/releases/solr/9.7.0/solr/core/src/java/org/apache/solr/search/similarities/LMJelinekMercerSimilarityFactory.java#L55-L56

Just use other ctor, avoid telescoping this boolean

cpoerschke · 2024-10-01T17:26:42Z

w.r.t. the CI failure -- https://github.com/apache/lucene/actions/runs/11130252581/job/30929198241?pr=13845 -- that locally seems to also happen on origin/main branch itself.

cpoerschke · 2024-10-02T10:04:04Z

w.r.t. the CI failure -- https://github.com/apache/lucene/actions/runs/11130252581/job/30929198241?pr=13845 -- that locally seems to also happen on origin/main branch itself.

merged in latest origin/main and the CI passes now then.

ChrisHegarty · 2024-10-02T16:29:37Z

The changes in this PR are to lucene.experimental classes. As such, these changes are suitable for a minor release. I would suggest to retarget to 10x, since 10.0.0 is well past feature freeze so only really accepting critical bug fixes at this point.

rmuir · 2024-10-03T01:36:18Z

lucene/core/src/java/org/apache/lucene/search/similarities/LMSimilarity.java

+  }
+
+  /** Creates a new instance with the default collection language model. */
+  public LMSimilarity(boolean discountOverlaps) {


just needs to be added to above "expert" ctor only.

rmuir · 2024-10-03T01:37:18Z

I added comments, this boolean parameter is esoteric and just needs to be available on "expert ctor": not added to every possible ctor for these classes. We just need to make it possible.

…ary constructor variants;

cpoerschke · 2024-10-03T09:13:30Z

I added comments, this boolean parameter is esoteric and just needs to be available on "expert ctor": not added to every possible ctor for these classes. We just need to make it possible.

Thanks for the review! Made updates accordingly.

…tructor variants, CHANGES.txt entries (#13845) (cherry picked from commit dab7311)

…tructor variants, CHANGES.txt entries (apache#13845) (cherry picked from commit dab7311) (cherry picked from commit cbd8b52) Resolved Conflicts: lucene/CHANGES.txt

…tructor variants, CHANGES.txt entries (#13845) (#13858) (cherry picked from commit dab7311) (cherry picked from commit cbd8b52) Resolved Conflicts: lucene/CHANGES.txt

…tructor variants, CHANGES.txt entries (apache#13845) (cherry picked from commit dab7311) Resolved Conflicts: lucene/CHANGES.txt

PR 13757 follow-up: add missing with-discountOverlaps constructor var…

ab513c9

…iants

cpoerschke commented Oct 1, 2024

View reviewed changes

cpoerschke marked this pull request as ready for review October 1, 2024 17:26

cpoerschke requested review from jpountz and rmuir October 1, 2024 17:27

cpoerschke mentioned this pull request Oct 1, 2024

similarities: provide default computeNorm implementation; remove remaining discountOverlaps setters; #13757

Merged

Merge remote-tracking branch 'origin/main' into pr-13757-follow-up

2115026

cpoerschke changed the title ~~PR 13757 follow-up: add missing with-discountOverlaps constructor variants~~ PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants Oct 2, 2024

add current and retrospective CHANGES.txt update

071eb03

cpoerschke changed the title ~~PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants~~ PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants, CHANGES.txt entries Oct 2, 2024

cpoerschke added this to the 10.0.0 milestone Oct 2, 2024

cpoerschke removed this from the 10.0.0 milestone Oct 2, 2024

rmuir reviewed Oct 3, 2024

View reviewed changes

cpoerschke added 2 commits October 3, 2024 10:04

action code review feedback: correct javadoc wording; remove unnecess…

af03b8d

…ary constructor variants;

update and move CHANGES.txt entry from 10.0 to 10.1 section

2543f23

cpoerschke requested a review from rmuir October 3, 2024 09:13

rmuir approved these changes Oct 3, 2024

View reviewed changes

cpoerschke merged commit dab7311 into apache:main Oct 4, 2024
3 checks passed

cpoerschke deleted the pr-13757-follow-up branch October 4, 2024 16:08

asfgit pushed a commit that referenced this pull request Oct 4, 2024

PR 13757 follow-up: add missing with-discountOverlaps Similarity cons…

cbd8b52

…tructor variants, CHANGES.txt entries (#13845) (cherry picked from commit dab7311)

javanna added a commit that referenced this pull request Oct 9, 2024

Move changes entry for #13845 to 10.0.0

111fc6f

javanna added a commit that referenced this pull request Oct 9, 2024

Move changes entry for #13845 to 10.0.0

767021e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants, CHANGES.txt entries #13845

PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants, CHANGES.txt entries #13845

cpoerschke commented Oct 1, 2024

cpoerschke commented Oct 1, 2024

cpoerschke Oct 1, 2024

cpoerschke Oct 1, 2024

cpoerschke Oct 1, 2024

rmuir Oct 3, 2024

rmuir Oct 3, 2024

cpoerschke Oct 1, 2024

rmuir Oct 3, 2024

cpoerschke Oct 3, 2024

cpoerschke Oct 1, 2024

rmuir Oct 3, 2024

cpoerschke Oct 1, 2024

rmuir Oct 3, 2024

cpoerschke Oct 1, 2024

rmuir Oct 3, 2024

cpoerschke commented Oct 1, 2024

cpoerschke commented Oct 2, 2024

ChrisHegarty commented Oct 2, 2024

rmuir Oct 3, 2024

rmuir commented Oct 3, 2024

cpoerschke commented Oct 3, 2024

PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants, CHANGES.txt entries #13845

PR 13757 follow-up: add missing with-discountOverlaps Similarity constructor variants, CHANGES.txt entries #13845

Conversation

cpoerschke commented Oct 1, 2024

cpoerschke commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpoerschke commented Oct 1, 2024

cpoerschke commented Oct 2, 2024

ChrisHegarty commented Oct 2, 2024

Choose a reason for hiding this comment

rmuir commented Oct 3, 2024

cpoerschke commented Oct 3, 2024