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

[cpoerschke]

No description provided.

[cpoerschke]

Not sure how best to javadoc(ument) this, suggestions welcome.

Also perhaps this could be a static method in the FieldInvertState class instead?

[rmuir]

let's avoid the static method

[rmuir]

Can we avoid doXXX and just give computeNorm a default implementation?

[cpoerschke]

Can we avoid doXXX and just give computeNorm a default implementation?

Sure, though where would the discountOverlaps value come from?

[rmuir]

the straightforward way would be to add:

• Similarity(boolean)
• Similarity() -> this(true)
• add getter (only), no setter. this is also duplicated in subclasses.

For me this stuff is ok, because we are making the tradeoff to simplify just the index-time side of this thing, by giving it a default implementation.

[rmuir]

I would also be in favor of adding some kind of warning/note, to the computeNorm: "expert: if you change this, index will not be different and not compatible with other similarities/require reindexing to change" and to the constructor taking the bool?

Idea is just that user extends Similarity and it "works", but other things are still possible

[cpoerschke]

the straightforward way would be to add:

• Similarity(boolean)
• Similarity() -> this(true)
• add getter (only), no setter. this is also duplicated in subclasses.

For me this stuff is ok, because we are making the tradeoff to simplify just the index-time side of this thing, by giving it a default implementation.

cbab8fb is a variation of that i.e. no changes to the constructor signature.

I'm still investigating w.r.t. some subclasses having setters and how that would interact with the constructor taking a boolean too.

[cpoerschke]

I'm still investigating w.r.t. some subclasses having setters and how that would interact with the constructor taking a boolean too.

91cfd5e is to have the flag and accessors in the base class, that's okay I think so long as the accessors are final (and the flag has private visibility).

[cpoerschke]

I would also be in favor of adding some kind of warning/note, to the computeNorm: "expert: if you change this, index will not be different and not compatible with other similarities/require reindexing to change" and to the constructor taking the bool?

Generally I like the idea of more context but am not sure how to make it very very clear e.g. when we say "if you change this" does that mean overriding-the-method or overriding-the-method-and-then-using-the-similarity-with-the-override-with-a-field-that-previously-used-the-original-similarity-to-use-the-custom-similarity or something else? Likewise when we say "not compatible/require reindexing" - is that works-but-gives-wrong-results or does-not-work-and-throws-exceptions or a variation of the two?

If (perhaps) there is a general assumption of "you cannot change the similarity of an existing field" and "similarity (and some other) changes require reindexing" then probably I would favour not having warnings or notes that convey that in some circumstances some changes can be compatible and don't require reindexing.

[rmuir]

Let's make this final (no need to set it to a value here as ctor always sets it) and remove the setter.

Otherwise, the way you had it without a boolean (abstract discountOverlaps) looked fine too?

But I think we shouldn't add mutability to the base Similarity class when setter is totally not needed by anyone.

[cpoerschke]

... when setter is totally not needed by anyone.

There is some residual setter use -- https://github.com/search?q=repo%3Aapache%2Flucene%20setDiscountOverlaps&type=code -- let me see what it would take to get rid of it.

[cpoerschke]

44c3f76 removes the residual setter use, seems a bit ugly but oh well.

[jpountz]

I like the simplification. Agreed on setting discountOverlap through the ctor rather than a setter.

[jpountz]

This looks good to me. @rmuir For my understanding, is there ever a good reason to set discountOverlaps to false?

[rmuir]

This looks good to me. @rmuir For my understanding, is there ever a good reason to set discountOverlaps to false?

The discountOverlaps is something we had from times where TF/IDF was the only scoring and index statistics were limited to docfreq() and maxdoc().

It is "easy" to understand: should the document's length be punished by synonyms? But at the same time, it makes it tricky to measure how well it is working, as the lucene user has the ability to easily inject a lot of artificial "synonym-like-things" in nearly infinite ways (e.g. word-delimiter-filters and stuff) with the analysis chain. So what would even be a fair measure?

Most of the modern scorers are doing something like BM25's dl/avgdl which makes this option harder to reason about. For example discountOverlaps still works in BM25 case: document doesn't get punished relative to other documents simply because it happened to have more synonyms or word-delimiters. But all documents get "skewed" in their scoring: this option only "discounts" the per-document dl, but does not impact the avgdl for the field (which is computed solely from term dictionary statistics). Since this is same skew to all documents: if you have tons of injected synonyms and are worried about it, you might want to try decreasing BM25's b parameter to adjust? Sorry, I haven't played too much with this, just general thoughts.

[jpountz]

Thanks, that makes sense. I'm wondering if we should remove the discountOverlap parameter from our similarities, and add support for it via a Similarity wrapper somehow, that would wrap/clone the FieldInvertState that is passed to computeNorm.

[rmuir]

Thanks, that makes sense. I'm wondering if we should remove the discountOverlap parameter from our similarities, and add support for it via a Similarity wrapper somehow, that would wrap/clone the FieldInvertState that is passed to computeNorm.

I don't like that this wacky index-time encoding is in Similarity class myself, it happens at a totally different time ("index time") than the rest of Similarity logic ("search time"), and you have to pass your Similarity to both IndexWriter and IndexSearcher, making it more confusing to the user and more complicated.

[rmuir]

I think it would be good to document what the default implementation is doing? e.g. at least it should state that it uses SmallFloat.intToByte4 lossy compression to encode the length as a single byte?

[cpoerschke]

I don't feel knowledgeable enough to properly document/edit on this.

"Allow edits and access to secrets by maintainers" is enabled for the pull request -- please feel free to (in-browser or otherwise) change the documentation here. Thanks!

[rmuir]

I took a stab at improving the docs in 47d4fa0

[rmuir]

I took another pass, I think it makes the Similarity class easier to use out-of-box? I think that modifying the index-time norm is more expert, especially after Adrien fixed the encoding to work better. It is still possible, but out-of-box you don't have to think about it as you have a default implementation?

[cpoerschke]

I took a stab at improving the docs in 47d4fa0

Thanks!

[cpoerschke]

#13845 to add some missing constructor variants.

[javanna]

This is missing a CHANGES entry. Did it also need to be added to the migrate guide? I added the 10.0 milestone as well while I was at it.

[cpoerschke]

This is missing a CHANGES entry. Did it also need to be added to the migrate guide? I added the 10.0 milestone as well while I was at it.

071eb03 added to #13845 to add CHANGES.txt entries.

Not sure if additional migrate guide steps are needed: the 8x to 9x steps -- https://github.com/apache/lucene/blob/releases/lucene/9.12.0/lucene/MIGRATE.md#bm25similaritysetdiscountoverlaps-and-legacybm25similaritysetdiscountoverlaps-methods-removed-lucene-9646 -- already contain something similar and it is relatively obvious that the setting moved to the constructor?

[javanna]

I did not realize that this change went into 9.12. I thought it was a 10.0 change. Sorry about the confusion.