Significant terms test refactor for extendability #75452
Conversation
benwtrent
added
>test
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
| public void testAssertions() { | ||
| testBackgroundAssertions(new ChiSquare(true, true), new ChiSquare(true, false)); | ||
| protected SignificanceHeuristic getHeuristic(boolean includeNegatives, boolean backgroundIsSuperset) { | ||
| return new ChiSquare(includeNegatives, backgroundIsSuperset); | ||
| } |
There was a problem hiding this comment.
...okay github, that's an interesting way to interpret that change
There was a problem hiding this comment.
yeah, sorry :(. It looked cleaner on a local git diff
There was a problem hiding this comment.
Not your fault. I think github optimizes for minimal edit distance, not fewest lines changed. I got the point though, and that's all that matters.
| import org.elasticsearch.search.aggregations.bucket.terms.heuristic.SignificanceHeuristic; | ||
|
|
||
| /** | ||
| * Abstract test case for testing significant term heuristics |
There was a problem hiding this comment.
Maybe a little more detail here? What makes the NXY version different than plain old AbstractSignigicanceHeuristicTestCase. What does NXY even mean here?
There was a problem hiding this comment.
I wish I knew what NXY meant, but that is what they are called...Will add some stuff.
There was a problem hiding this comment.
:lolsob: Well, at least I'm not the only one.
|
|
||
| /** | ||
| * @param includeNegatives value for this test run | ||
| * @param backgroundIsSuperset value for this test run |
There was a problem hiding this comment.
Again, I think a little more detail would be helpful. Think about reading this as a new developer coming to this code. Include negative what? What does backgroundIsSuperset mean? Superset of what?
There was a problem hiding this comment.
backgroundIsSuperset is that its the super set of the "subset". Otherwise they are treated as two disjoint sets. Will add some better javadocs.
The original PR elastic#75264 made some test mistakes NXY Significant term heuristics have additional values that need to be set when testing basicScore properties. Additionally, previous refactor kept the abstract test class in a package that other plugins don't have access to. closes elastic#75442, elastic#75561
The original PR #75264 made some test mistakes
NXY Significant term heuristics have additional values that need to be set when testing
basicScore properties.
Additionally, previous refactor kept the abstract test class in a package that other plugins
don't have access to.
closes #75442